feat: perform background fetch from all transports - #8592
Conversation
50704f9 to
7720b34
Compare
So every background fetch call will always go the full timeout if one of your relays on one of your profiles is offline? In that case android also needs to be updated as I think they also synchronously wait on this function (albeit only for 10 seconds) |
Offline relays will typically immediately fail, so there is no waiting for that. But if you implement concurrent event processing during background fetch, you shouldn't have to worry. The first working relay delivering a message will give you an incoming message, no matter what other connections hang or still need time. |
7720b34 to
6c31d9e
Compare
6c31d9e to
799ad94
Compare
799ad94 to
7ebd4f5
Compare
|
Tested these happy paths on iOS:
* If you received different messages on different relays since last fetch the fastest relay will decide the messages user is notified about |
|
Found an issue: Details
Receiving two messages (one or two seconds apart) while main relay is blocked results in one immediate notification and one after the timeout delay (on a second NSE run). I suspect that this is due to not keeping all the relay connections open until the end of the timeout? So I think the successful relay is closed before the hanging relay is canceled. Suspected timeline:
Edit: This is now resolved in this PR for multi relay profiles but still exists between single relay profiles where one is down. That is not a regression however since that is how it works on main currently too. |
1c908cb to
1c41bfe
Compare
With I/O stopped, `background_fetch()` connected only to the transport of `configured_addr` and we now instead fan out to all transports in a controlled loop. If a first transport finished fetching new messages cancel all other attempts and return. This is meant to address the problem that amzd described where a profile with one functioning and one hanging transport, shows the first notification, then hangs 15 seconds waiting for the hanging transport. meanwhile a second NSE arrives and dies, and the second message is not notified or only generically. Also drop the quota check from this background fetch path: its result is in-memory only, discarded when the iOS notification service exits, and the regular scheduler fetching refreshes it every 60s anyway. Moreover, quota errors/running full is pretty rare since relays generally automatically stay under quota these days. It's another round trip for each transport of each profile and simply not necessary. Also adds previously missing online tests.
A second background_fetch() while one is already running returned without emitting the event, and the FFI returned 1 for it, so a UI waiting for an event hangs dc_get_next_event(). Emit the event in any case, so waiting for it is safe.
|
Tested and working well. Since the last change it is no longer that important for the UIs to update since this PR now actually makes it less likely for background fetch to time out (for multi relay profiles) because it only waits for the first relay to complete with new messages. |
7219c7c to
da42f27
Compare
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
Some minor things, and a concern whether it's fine cancel the fetching (maybe it's fine, I'm unsure there).
FTR, (according to my understanding to async rust and scripts/future-sizes.sh), this will increase RAM usage by 23kb per extra transport: 5kb for the fetch_from_transport() future, and 17kb for the fetch_new_msg_batch() future, which is created for every transport. It's only inside fetch_new_msgs_batch() that the fetch_msgs_mutex is locked, which means that the future is created for every transport, even while it's not fetched from.
We could move the Box::pin from around fetch_new_msg_batch() to around fetch_many_msgs() in order to bring the extra RAM usage down to ~5-10kb per transport, but it should be good as-is already, and we'll anyways need to properly look into the future sizes at some point.
| /// Returns as soon as one transport fetched messages and drops the fetches | ||
| /// still in flight, so that a caller woken up by a push notification | ||
| /// does not wait for a transport that may never answer. | ||
| pub(crate) async fn fetch_from_all_transports(&self, context: &Context) -> Result<()> { |
There was a problem hiding this comment.
Hm, I'm not yet sure whether dropping the in-flight fetches is a good idea here. Maybe it's fine, but there will be a lot more cases where receive_imf() is cancelled, and we already know that this can cause problems.
This will be a typical background fetch when there are three transports
- All three transports do prefetch simultaneously, transport 1 finishes first, the other two finish shortly thereafter.
- Transport 1 takes the lock and fetches messages.
- Transport 1 finishes fetching and calls delete_expired_imap_messages() and move_delete_messages(). At the same time, transport 2 gets the lock and starts fetching any messages that were not available on transport 1.
- Transport 1 is done, and cancels the ongoing fetch that is done by transport 2.
I see two options that could solve this problem, if we deem it solve-worthy.
The first option is expanding the existing logic in fetch_new_msg_batch(), the second option is writing a new function that does exactly what we need for background fetch:
-
First option: A boolean parameter
only_fetch_from_one_transport(or similar) is passed through tofetch_new_msg_batch. Then, if one of the transports downloaded messages, before releasing the lock it sets a context-global flag (it can live inside the lock). The other transports check this flag right after taking the lock, and if it's set then they return immediately. -
Second option: Move the call to
prefetch()out offetch_new_msg_batch()up intofetch_new_messages().Then,
fetch_from_all_transports()doesn't callfetch_move_delete(). Instead, it directly callsprepare(),select_with_uidvalidity()andprefetch()simultaneously for all transports,
and the transport that finishes first is used forfetch_new_msg_batch(),interrupt_ephemeral_task(),delete_expired_imap_messages(), andmove_delete_messages().
| return [msg for msg in chat.get_messages() if msg.get_snapshot().text == text] | ||
|
|
||
|
|
||
| def wait_for_imap_message(imap, timeout=60): |
There was a problem hiding this comment.
Some runners are randomly paused in CI (according to link2xt, who knows our CI better than I do), which means that a 60s timeout can be too short and lead to unnecessary flakiness. Instead, it's better not to have a timeout here. The global pytest timeout is enough, and it is is easily configurable across all tests rather than being different per test.
| # Leave the message on the second transport only. | ||
| imap1.delete("1:*") | ||
|
|
||
| dc.background_fetch(30) |
There was a problem hiding this comment.
Same comment about the timeout
| dc.background_fetch(30) | |
| dc.background_fetch(300) |
| for transport in alice.list_transports(): | ||
| wait_for_imap_message(direct_imap(alice, transport["addr"], transport["password"])) | ||
|
|
||
| dc.background_fetch(30) |
There was a problem hiding this comment.
Also here
| dc.background_fetch(30) | |
| dc.background_fetch(300) |
4 participants
With I/O stopped,
background_fetch()connected only to the transport ofconfigured_addr(see #8572) and we now instead fan out to all transports once in a controlled loop without any notion of primary. This createsNIMAP connections withN=num(all_transports_of_all_profiles), instead ofN=num(all_profiles). Whichever connection for each profile wins, will fetch and lock out the other connections, so that RAM usages through fetching actual IMAP messages should not change.Known usages
background_fetchin UIs:Android permanent background fetch (timeout 300s): https://github.com/deltachat/deltachat-android/blob/49aa4ab189e7ab61441aaef14a9ad9249e44c050/src/main/java/org/thoughtcrime/securesms/service/FetchForegroundService.java#L73-L82
Android FCM background fetch (timeout 10s): https://github.com/deltachat/deltachat-android/blob/49aa4ab189e7ab61441aaef14a9ad9249e44c050/src/main/java/org/thoughtcrime/securesms/service/FetchForegroundService.java#L88-L106
iOS NSE background fetch (timeout 15s):
https://github.com/deltachat/deltachat-ios/blob/64804f6c467faedabfd7b77b1f9538c3f6b7d201/DcNotificationService/NotificationService.swift#L5-L8)
Deltatouch (UbuntuTouch) background fetch (timeout 4s) https://codeberg.org/lk108/deltatouch/src/commit/ac45eb90ede50818df00583b9111fbe26991e460/package/click/pushhelper.cpp#L223
With this PR, the overall behaviour should already improve, as we now use the first transport that connects fastest during
background_fetch, instead of relying on a "primary" one only. However, current callers would still wait for all transports of all profiles to finish their single background fetch-round with the respective timeouts.This can not be fixed in core, but UIs could drain events concurrently during background fetch, and cancel any fallback-handling if a real notification arrives. This should kind of guarantee UIs see the first message from whichever transport and whichever profile it arrives, and do not have to wait to the end of all background fetches (which might timeout, be killed by the OS etc.).
The PR also drops the
Nquota checks from background fetch: the check result is in-memory only, discarded when the iOS NSE exits, and the regular scheduler fetching refreshes quota every 60s anyway. Moreover, quota full are pretty rare since relays generally automatically stay under quota these days. This savesNconcurrent round trips during background fetch and is simply not necessary.Also adds previously missing online tests.