Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve WebUI settings #2243

Merged
merged 8 commits into from
Jan 13, 2022
Merged

Improve WebUI settings #2243

merged 8 commits into from
Jan 13, 2022

Conversation

stickz
Copy link
Collaborator

@stickz stickz commented Dec 31, 2021

This pull request greatly improves the process of loading and saving WebUI settings. I originally created PR #2239, but decided to rethink how to implement this improvement. There are no race conditions here and it doesn't fix anything that is not broken.

List of Changes:

  1. In rtorrent.js the getsettings.php AJAX request is changed from a POST method to GET method. We are getting data from the server not sending data. Flagging the method as POST is not the correct way of completing this request.

  2. The WebUI now sends an asynchronous request to retrieve the UI settings that are stored on the web server. This reduces the TBT (Total Blocking Time) of the main thread before it's time to finish initializing the WebUI. The WebUI will not finish initializing until this task has completed, to ensure there's no untended side effects with saved settings or initializing plugins.

  3. theWebUI.config() no longer contains the data from the getsettings.php AJAX request as a function parameter. None of the default ruTorrent plugins currently require this information. No third party plugins will require this information ether. A duplicate copy is stored in theWebUI.settings array. All of the default plugins have been updated to reflect this change.

It is very important to implement this change because it prevents the possibly of a memory of a leak being created. It also prevents plugins from accidentally not sending this information back to the WebUI and causing bugs with loading settings.

  1. There is one bug with loading WebUI settings which has been resolved with this pull request. The wrong WebUI settings were being loaded because of a race condition with writing to uisettings.json on the web server. This problem is now fixed.

  2. There is a new WebUISettings class for reading and writing WebUI settings on the web server. The WebUI settings are now stored in WebUISettings.dat instead of uisettings.json. They are loaded and saved by the rCache class instead.

  • The WebUI settings are now cached into memory instead of being repeatedly read from the disk. This reduces overhead.
  • The WebUI settings are no longer written to the disk unless the JSON string changes. This is possible due to cached loading.
  • There is no longer a race condition when writing the WebUI settings to the disk. rCache will safely usleep the thread until the file lock is removed. The previous behavior was not to update the WebUI settings, if the file was locked.

Future Improvements:
If this pull request is merged, it will allow for future improvements to be implemented for the WebUI settings. I did not include them with this pull request. It's better to have a smaller pull request to make code review and testing easier for everyone.

  1. It will be possible to finish initializing the WebUI, even if the request to retrieve the WebUI settings from the web server fails.
  • A translated warning can be displayed to the user, so they can attempt to correct the problem.
  • The WebUI can temporarily load with the default settings, so the user can still use ruTorrent.
  • The WebUI settings can be prevented from saving settings. This way, the user will not lose their originally saved settings.
  1. It will be possible to only POST the changed value to the web server, when a WebUI setting is updated. This will greatly reduce the AJAX request overhead for setsettings.php. It will be possible to send multiple async requests at once without having to worry about the overhead. Also, a larger JSON string will be possible. This will make future improvements practical.

@stickz
Copy link
Collaborator Author

stickz commented Jan 2, 2022

I just fixed an issue with the WebUISettings not loading properly on the first load. It sends an empty JSON string the first time settings load. The loading routine was expecting a value for "webui.lang" inside the JSON string. I had to add a null check.

@Novik
Copy link
Owner

Novik commented Jan 9, 2022

What about compatibility with this ?

@stickz
Copy link
Collaborator Author

stickz commented Jan 9, 2022

What about compatibility with this ?

I will make this pull request compatible with PR #2247. I just have to update a couple files to use the new utility classes. It may take up to four days to complete this task. I have to return to work within one day.

js/webui.js Outdated Show resolved Hide resolved
@stickz
Copy link
Collaborator Author

stickz commented Jan 12, 2022

I fixed setting of the webui lang in the last commit. I also resolved anther regression with trying to configure the webui multiple times. This was firing setsettings.php unnecessarily and was causing an overflow when trying to open the settings menu.

js/webui.js Show resolved Hide resolved
js/webui.js Outdated Show resolved Hide resolved
@Novik Novik merged commit c555ae6 into Novik:master Jan 13, 2022
@stickz stickz deleted the WebUISettings branch January 13, 2022 19:57
stickz added a commit that referenced this pull request Sep 19, 2022
Fixes regression with PR #2243.

The 'theWebUI.config' no longer takes the 'data' argument. We use the 'this' keyword to call the stored base function from current execution context. We do not call it from the global theWebUI object. These steps prevent memory leaks in the web browser.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants