add Background Fetch method

[Simon-Laux]

• refactor: move convert folder meaning logic in own method
• feat: add background fetch method
• api: jsonrpc: add background_fetch_for_all_accounts
• api: cffi: add dc_accounts_background_fetch_with_timeout

closes #4420

Todo:

• limit how often quota is checked
• BackgroundFetchCompletedForAllAccounts event as described in use core background fetch deltachat-ios#2017 (comment) (basically to provide a way to know when all events caused by the background fetch were processed)

[r10s]

maybe the question from deltachat/deltachat-ios#2017 (comment) is better places here, as more related to core than UI:

when does dc_accounts_background_fetch_with_timeout() return? is that comparable to when dc_all_work_done() (which can be removed btw) returns true?

asking as processing is done by events which handlers may still run if dc_accounts_background_fetch_with_timeout() returns much sooner that dc_all_work_done() is fired (not saying that for for dc_all_work_done() things are perfect wrt timing, just want to bring up that aspect)

[Simon-Laux]

I answered over at deltachat/deltachat-ios#2017 (comment):

is that comparable to when dc_all_work_done() (which can be removed btw) returns true?

yes, maybe even slightly faster because it does less. It always takes as long as the slowest account, so 1-8 seconds in my test. (the riseup provider was sometimes slow, the average/normal time of the other providers was sth. like 2 seconds)

asking as processing is done by events which handlers may still run if dc_accounts_background_fetch_with_timeout() returns much sooner that dc_all_work_done() is fired (not saying that for for dc_all_work_done() things are perfect wrt timing, just want to bring up that aspect)

There is still the sleep second after, that should give enough time to process all events (empty the queue), but we could also do a semaphore to empty the event queue to be absolutely sure. Good catch, though also note that relying on the connectivity event which changed behaviour brought us the bug in the first place, so I would be careful about relying on events like that (that we could break by accident again).

We could also make a new BackgroundFetchCompletedForAllAccounts event that the core function could add to the event queue at the end / or when it fails, then we could have a semaphore for reaching that exact event and don't need any additional core check "if work is already fully done".

Originally posted by @Simon-Laux in deltachat/deltachat-ios#2017 (comment)

[r10s]

one question: DC_EVENT_BACKGROUND_FETCH_COMPLETED_FOR_ALL_ACCOUNTS works as it is added to the end of the event queue after all other events are emitted? (background was the the question if all events are processed when dc_accounts_background_fetch() returns)

[Simon-Laux]

one question: DC_EVENT_BACKGROUND_FETCH_COMPLETED_FOR_ALL_ACCOUNTS works as it is added to the end of the event queue after all other events are emitted?

yes, that's the purpose of this event.

[r10s]

better use the same, consistent method names as in ffi:

Suggested change

	async fn background_fetch_for_all_accounts(&self, timeout_in_seconds: f64) -> Result<()> {
	async fn accounts_background_fetch(&self, timeout_in_seconds: f64) -> Result<()> {

(i see that the the "for_all_accounts" suffix is already in use, however, it is a little cumbersome wording and maybe not worth to be set in stone further)

[r10s]

nice explanation!

[r10s]

this function seems not to be needed as public; and making it public overcomplicates things:

apart from documentation and maintainance effort, eg. the AccountsBackgroundFetchDone event seems to be emitted twice (background_fetch_with_timeout calls background_fetch() and both emit AccountsBackgroundFetchDone at the end)

[Simon-Laux]

I See the following possibilities:

• A. only emit the event in background_fetch_with_timeout
• B. conditionally emit the event in background_fetch_with_timeout only on timeout
• C. merge both functions into one (require to have a timeout)
• D. same as C, but keep it in two functions, making the background_fetch private / internal

As this is about networking, and it is in the nature of networking to be flaky, I'd say there is no really a use case for calling the background fetch without a timeout, so I think we should go for C (or D if we think we will find a use case for reusing the function in the future).

[r10s]

yip, let's go for C. we currently do no need the case of "no timeout" and this is also not forseeable. yagni :)

[link2xt]

I pushed a commit to fix this (D variant).

[r10s]

maybe @link2xt can have a final look at the pr as well, also wrt future maintenance, he's most involved in.

to sum up, the idea of the PR is to make things more reliable than the old call to maybeNetwork() (that sets connectivity to "connecting") followed by a wait for DC_EVENT_CONNECTIVITY_CHANGED where allWorkDone() is checked. tbh, there are no clear indications that the old method is unreliable, but indeed, it appears implicit and fragile (although i just noticed there seems to be a test)

the new approach of this PR is a dedicated accountsBackgroundFetch() that emits DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE.

currently, both approaches are for iOS mainly, but that may change in future.

UI-wise it is a very easy replacement, most load is in core :)

[link2xt]

I have rebased and pushed one commit on top to fix the test with a note why warn! is fine. We still don't have Sent folder one the CI server and probably don't want it, the test checking that nothing critically fails if Sent folder does not exist is also fine as it is.

[link2xt]

tbh, there are no clear indications that the old method is unreliable, but indeed, it appears implicit and fragile (although i just noticed there seems to be a test)

There is a test after fixing it once, but the need to understand the whole "all_work_done" and how it is used by iOS when you just want to change connectivity is not nice, better have connectivity view as something user-visible but not machine-readable.

[Simon-Laux]

tbh, there are no clear indications that the old method is unreliable

It is not only about an easier more direct api, it is also to give us full control in core about what we do and also doing less work than with the normal imap loop. It is kinda like what dc_send_msg_sync is for dc_send_msg.

[r10s]

it is also to give us full control in core

one idea/question that came up while i explained the PR to @hpk42 : the initial idea of having a blocking call is a bit faded out by the need to wait for the event DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE in UI.

things would be easier in UI and more under control of core, if that is not needed. also, as dc_background_fetch() will probably also become handy on android at some point.

so question is: what would be the effort if core waits at the end of dc_accounts_background_fetch() until dc_get_next_event() has returned DC_EVENT_ACCOUNTS_BACKGROUND_FETCH_DONE (or so, no need to expose the event in that case)

we should at least consider this question, might be it is far too much effort to do that in core and the UI way is much easier.

[r10s]

fine for me :)

EDIT: i just restarted the failing test to see if it is "real" :)

[link2xt]

Failing test is #5201