Allow pausing IO scheduler from inside core #4138
Conversation
|
one question is also whether we should completely remove the |
There was a problem hiding this comment.
great stuff!
really looking forward to have that in! eg. UI typically call startIO on periodically every some minutes because of wakeups - or when the app comes to foreground. in the past, they have to take care not to do that under some circumstances - which is hard to track - and also was just not done in many cases.
however, although code looks good on a first glance (i esp. checked some boolean conditions) i cannot really judge the thing, i do know too less about the rust internals here.
isn't it still needed on shutdown? otherwise, yes, i'd do a stub as |
oh, I was only going to remove it from the rust lib. not from deltachat-ffi which can keep it as a no-op. |
desktop still uses all start and stop io calls (on manager and on account) to achieve the feature of only syncing the active account instead of all accounts in the background. But in the future it would be nicer if core could also manage this scenario. (desktop uses it over the jsonrpc api, so you could make the cffi a no-op probably?) |
Why is it needed? I thought it is better to have all accounts synchronized in the background so when you go to an account it is already updated. |
ok, let's not touch this for now. no harm leaving stop_io in. |
|
@link2xt any remaining problems before this can be approved? would be nice to get merged |
src/scheduler.rs
Outdated
| if self.done { | ||
| return; | ||
| } | ||
| let context = self.context.clone(); |
There was a problem hiding this comment.
Maybe emit an error! here and do nothing else? resume is better called explicitly in the async function so we don't have to spawn a task here. Spawning tasks from Drop is tricky, maybe everything around is in the process of being dropped. We normally don't use this, so this is just some untested dead code that may become unmaintained and not actually working when needed.
There was a problem hiding this comment.
I tested that when panic! is inserted here the tests still pass, then replaced it with error!.
To handle backups the UIs have to make sure they do stop the IO scheduler and also don't accidentally restart it while working on it. Since they have to call start_io from a bunch of locations this can be a bit difficult to manage. This introduces a mechanism for the core to pause IO for some time, which is used by the imex function. It interacts well with other calls to dc_start_io() and dc_stop_io() making sure that when resumed the scheduler will be running or not as the latest calls to them. This was a little more invasive then hoped due to the scheduler. The additional abstraction of the scheduler on the context seems a nice improvement though.
|
I have rebased the PR |
I checked that tests still pass even if error! is replaced with panic!
|
I have resolved my own comments and will merge this manually with |
To handle backups the UIs have to make sure they do stop the IO
scheduler and also don't accidentally restart it while working on it.
Since they have to call start_io from a bunch of locations this can be
a bit difficult to manage.
This introduces a mechanism for the core to pause IO for some time,
which is used by the imex function. It interacts well with other
calls to dc_start_io() and dc_stop_io() making sure that when resumed
the scheduler will be running or not as the latest calls to them.
This was a little more invasive then hoped due to the scheduler. The
additional abstraction of the scheduler on the context seems a nice
improvement though.