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

rss: Fix timer sync #2569

Merged
merged 2 commits into from
Dec 29, 2023
Merged

rss: Fix timer sync #2569

merged 2 commits into from
Dec 29, 2023

Conversation

TrimmingFool
Copy link
Contributor

@Antorell Does the timer work as intended with this fix?

The timer now indicates when the next loadRSS will be called.

Fixes #2568

@Antorell
Copy link

Antorell commented Sep 9, 2023

I'm gonna try the fix now.

@Antorell
Copy link

Antorell commented Sep 9, 2023

Same problem. One thing I didn't notice is rutorrent grabs releases at the same time rss/update.php triggers, 45sec before the timer and the update of the listing on the webui.

root@ubuntulocal:/var/www/html/ruTorrent# git apply --verbose 2569.patch
Checking patch plugins/rss/init.js...
Checking patch plugins/rss/rss.php...
Checking patch plugins/rss/init.js...
Applied patch plugins/rss/init.js cleanly.
Applied patch plugins/rss/rss.php cleanly.
Applied patch plugins/rss/init.js cleanly.

Screenshot (15)

edit: clarification

@TrimmingFool
Copy link
Contributor Author

TrimmingFool commented Sep 9, 2023

Same problem.

@Antorell On the system I tested it fixed the problem. It is unlikely that nothing has changed, did you clear the browser's cache? Otherwise it would be helpful to know which calls occur on your system by adding some logging

(at plugins/rss/init.js:244) console.log(new Date(), 'loadRSS', new Date(theWebUI.rssUpdateTimestamp), theWebUI.rssTimeUntilNextUpdate());`
(at plugins/rss/init.js:213) console.log(new Date(), 'rssTimeUntilNextUpdate', new Date(theWebUI.rssUpdateTimestamp));

One thing I didn't notice is rutorrent grabs releases at the same time rss/update.php triggers, 45sec before the timer and the update of the listing on the webui.

Since there is no push mechanism (such as SSE or WebSocket) to signal the frontend that the feeds have completed loading, it is assumed that downloading and parsing the feeds takes no longer than 45 seconds. After this delay the frontend calls loadRSS to fetch this result. This behavior has not changed. What has changed is that the timer should now show the time until loadRSS is called.

@Antorell
Copy link

Antorell commented Sep 9, 2023

The cache is cleared yes. The cache of the browsers I use always cleared anyway as it's set to delete the cache when the browsers close, and edge is set to shutdown/not remain in memory on close.

Since there is no push mechanism (such as SSE or WebSocket) to signal the frontend that the feeds have completed loading, it is assumed that downloading and parsing the feeds takes no longer than 45 seconds. After this delay the frontend calls loadRSS to fetch this result. This behavior has not changed. What has changed is that the timer should now show the time until loadRSS is called.

Maybe we weren't looking at the same issue. The thing that bugged me the most was the triggering of update.php and grabbing of releases was ~45 seconds before the reset of the timer (and refresh of the listing on the webui). If the 45 seconds difference is/was hardcoded, no wonder I didn't see any difference when I patched rutorrent with your PR. Thanks for exposing/making this hardcoded timeout a bit more obvious.

I tried with a 10 sec delay (expectedFetchDelay = 45 * 1000; ) on my currently overloaded seedbox, it's a relic 4 cores Xeon from 2011 with 8GB ram on a ~1Gbps line, and when update.php was triggered, it grabbed a file then reset the timer and loaded/refreshed the RSS listing with new releases a handful of seconds later. A 45 seconds timeout to assume the feed loaded correctly is a very very long time nowadays with Gigabit(s)/Gbps internet speeds and fast machines.

Thanks again for making this timeout more obvious. I'll just keep on editing (expectedFetchDelay = 45 * 1000; ) to something lower for sanity (mine) sake.

@TrimmingFool
Copy link
Contributor Author

A 45 seconds timeout to assume the feed loaded correctly is a very very long time nowadays with Gigabit(s)/Gbps internet speeds and fast machines.

45 seconds as an upper bound is not that unreasonable since there are some networks that are not that fast. If the greatest expected delay was lower than this, the socket connection timeout (currently at 30s) might also need adjusting.

var $_fp_timeout = 30; // timeout for socket connection

However, I think, it it would be best to leave it as it is, right now.

@stickz
Copy link
Collaborator

stickz commented Sep 18, 2023

Target set for ruTorrent v4.3. This will need to go into develop. I will rebase in the near future. I'm freezing the v4.2 branch for critical fixes only. It's stable now and we should avoid regressions at all costs.

@TrimmingFool TrimmingFool changed the base branch from v4.2 to develop November 28, 2023 12:25
@stickz
Copy link
Collaborator

stickz commented Dec 29, 2023

@TrimmingFool Ready to merge into develop when conflicts are resolved.

Copy link
Collaborator

@stickz stickz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate variables need resolved before merging.

init.js for RSS has 1700 lines of code. Tasks are being performed that do not directly initialize the interface. I will merge this PR since it doesn't contribute to the problem.

Could we possibly make a new class for parsing bbcode to html and transforming it? This would allow us to further develop the RSS feeds feature on ruTorrent. It's very difficult to debug right now and theWebUI object is polluted.

plugins/rss/init.js Outdated Show resolved Hide resolved
plugins/rss/init.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants