feat: Make the IoPausedGuard a simple sender #4184
Conversation
This makes the BackupProvider automatically invoke pause-io while it is needed. It needed to make the guard independent from the Context lifetime to make this work. Which is a bit sad.
This replaces the mechanism by which the IoPauseGuard makes sure the IO scheduler is resumed: it really is a drop guard now by sending a single message on drop. This makes it not have to hold on to anything like the context so makes it a lot easier to use. The trade-off is that a long-running task is spawned when the guard is created, this task needs to receive the message from the drop guard in order for the scheduler to resume.
src/scheduler.rs
Outdated
| /// | ||
| /// Does nothing special, simply drops this guard. | ||
| pub(crate) fn resume(self) { | ||
| drop(self) |
There was a problem hiding this comment.
arguably we could remove this method. i couldn't make up my mind so feel free to have an opinion.
|
Maybe not entirely related, but what happens if you have overlapping pause guards?
Shouldn't the guard also hold the lock on the scheduler place, so it cannot be started? |
|
Btw, why do we have pause guards instead of "run guards" locking some mutex in the scheduler? So, when we start IO, we lock that mutex and monitor whether we're requested to stop IO and release the mutex. And then try to acquire it again if the program isn't requested to terminate / a user didn't issue the |
|
We don't even care about I/O start/stop, but a general write lock on the storage, the database plus blobdir. E.g. keeping IMAP connections is fine if we don't download messages, there is no need to close them. If we acquire a write lock when entering the "fetch messages" procedure and release it before going to idle, we don't need to stop I/O at all. Not sure what the situation on iOS is, but I expect that it is not required to close all sockets before going to background sleep. So maybe with #4165 we don't even need to stop the I/O to take database snapshot, and if we don't modify or garbage-collect the blobs while running, we can even keep using the database while setting up a new device. |
|
IIUC IOS does never stop IO and only starts IO whenever it is woken up, brought to the foreground or similar just to make sure things are really running. |
Good catch, I created a #4193 separately as it's not really what this PR tries to address. |
|
I'd also like to use this PR as a model to improve the ongoing process API, which is simpler anyway as there can only be one. So what do we think about the merits of this implementation? Is it an improvement? I think it is. |
src/scheduler.rs
Outdated
| /// | ||
| /// Does nothing special, simply drops this guard. | ||
| pub(crate) fn resume(self) { | ||
| drop(self) |
| } | ||
| let (tx, rx) = oneshot::channel(); | ||
| tokio::spawn(async move { |
There was a problem hiding this comment.
Btw, why can't it be spawned in drop()?
There was a problem hiding this comment.
https://docs.rs/tokio/latest/tokio/fn.spawn.html#panics
If you happen to drop this outside of the runtime, which you have effectively no control over, things start going wrong. This could potentially happen at shutdown time. By spawning here you have guaranteed correct semantics even when things are being torn down.
If you want to dive deep into this there's a super long essay exploring async drop that I still haven't read myself: https://sabrinajewson.org/blog/async-drop
There was a problem hiding this comment.
Btw, another approach is to make a generic async function that:
- Pauses IO
- Awaits a future passed to it
- Resumes IO
- Returns the result of the awaited future
This way no async drop is needed.
There was a problem hiding this comment.
true, callback based APIs are often more awkward to use though
This replaces the mechanism by which the IoPauseGuard makes sure the IO scheduler is resumed: it really is a drop guard now by sending a single message on drop.
This makes it not have to hold on to anything like the context so makes it a lot easier to use.
The trade-off is that a long-running task is spawned when the guard is created, this task needs to receive the message from the drop guard in order for the scheduler to resume.
#skip-changelog
Targets
flub/backup-provider-pause-ioto minimise rebasing. Should be merged independently after #4182.@link2xt what do you think about this approach to async drop guard? It is a lot simpler to manage. I don't think it ever runs into issues of things not being alive when the world is dropped. The main thing is that it relies on a long-running task, I don't have any issues with this but my only worry is that phones might kill our executor in funny ways? Is it possible for the spawned task to disappear without the
Contextitself being destroyed? I guess not since we'd be running into all sorts of issues if our spawned tasks disappear.