Delay refreshAllSubscriptions after timer deadline#2084
Delay refreshAllSubscriptions after timer deadline#2084barijaona wants to merge 2 commits intoViennaRSS:masterfrom
Conversation
|
Vaguely inspired by 0f44c88 |
|
I added a commit to check for network availability. I think there were two pitfalls with your commit:
I think checking for network availably improves on this. It might be enough to resolve #2005 without making this too complicated for now. |
|
I reckon there is a problem related to manual refreshes in my initial commit. @Eitot's additional commit behaves correctly even on macOS 26, but in fact it depends heavily on SCNetworkReachability, which makes it fragile as these functions are deprecated. I'll push an alternative correction. |
|
nw_path_monitor is the replacement for SCNetworkReachability, but not available for macOS 10.13. I might have a look as well to see if there is an equivalent function. |
7b81157 to
1f60381
Compare
| if (refreshImmediately){ | ||
| [self refreshAllSubscriptions]; |
There was a problem hiding this comment.
This code path gets executed if the user enabled refreshOnStartup and automatic fetching. This means that your fix isn't applied.
There was a problem hiding this comment.
This is not really a problem, as the issue we have (in at least 99% of real life cases) is with wakes from sleep, which lead the system to re-establish network connections.
This part of code would be executed in the aftermath of a manual launch of Vienna by the user: in most of cases, whether the network has already been established, or you have a more permanent problem which might require another intervention, and just waiting a little more would not help.
There was a problem hiding this comment.
The entire block is executed whenever the timer fires. refreshImmediately is only set on launch or when the user changes the refresh frequency in settings. It makes no sense to refer to it within the inner block because its value will realistically never change. As I said, if refreshImmediately is set to YES (e.g. on launch), then the other code path – and therefore your fix – is simply never executed.
| } else { | ||
| // the time offset is for allowing network reactivation | ||
| // after wakeup from sleep | ||
| [self performSelector:@selector(refreshAllSubscriptions) | ||
| withObject:nil | ||
| afterDelay:20.0]; | ||
| } |
There was a problem hiding this comment.
This applies the delay to every automatic fetch, even for those users that have a good network connection.
There was a problem hiding this comment.
Yes, but as the delay is applied for all automatic launches, subsequent intervals are respected.
Anyway, the offset is within the 10% leeway we have given to the timer.
Let's note that this code is inspired the logic that was implemented in year 2006 by commit 0f44c88 and went away in 2020 as an unexpected consequence of modernization in commit 03a58f0
There was a problem hiding this comment.
So I consider this is good enough and I tend to think that less code means less maintenance 🙂
There was a problem hiding this comment.
The old 2006 code you refer to was only executed on wake-up, so this delay didn't generally apply to every automatic fetch.
I just don't find this elegant and there is no guarantee that a hard-coded delay even reliably fixes the bug. Whether 20 seconds is enough is ultimately just a guess.
|
@barijaona: A minimal implementation of a network monitor based on the Network framework (10.14+): https://gist.github.com/Eitot/6ce39a604b67be34036d83d9b25d9e4b |
Work around issue ViennaRSS#2005 On system wake, Vienna should wait a couple of seconds until the network connection becomes available before refreshing feeds
To respond to user actions, calling directly refreshAllSubscriptions is simpler than relying on the timer. We just have to reschedule next invocations. I kept the fire() function in DispatchTimer, even though we don't use it.
1f60381 to
7e8d83a
Compare
Work around issue #2005 "On system wake, Vienna should wait a couple of seconds until the network connection becomes available before refreshing feeds"