deltachat-core-rust
deltachat-core-rust copied to clipboard
deltachat
ref: make Context::alloc_ongoing return a guard
This guard can be awaited and will resolve when Context::stop_ongoing is called, i.e. the ongoing process is cancelled. The guard will also free the ongoing process when dropped, making it RAII and easier to not accidentally free the ongoing process.
#skip-changelog
This also found a real bug: https://github.com/deltachat/deltachat-core-rust/pull/4249
I am completely stumped by the test failure since merging master... it's not a flaky failure.
@flub Could you rebase it or redo it on top of the current stable? I am going through oldest PRs and this one seems to be stale.
@flub Could you rebase it or redo it on top of the current
stable? I am going through oldest PRs and this one seems to be stale.
I guess now this PR also should go to main instead.
I looked a bit at the failures now. And while I could fix them I'm a bit unsure how to proceed:
The failures occurring are because async drop is difficult. So this PR works around that by spawning a task when the drop guard is created and the drop guard itself only sends a message to that task to do run the drop impl.
The major problem with this approach is that drop is no longer deterministic. It could run any other time. And this is what makes the tests falky now: sometimes drop has run, sometimes it hasn't and the context is not yet freed.
One could argue this is a matter of more synchronisation: I can add code that sends a signal when drop is finished and we can await until the mutex is truly freed. However this makes using it again brittle: because it is easy enough to write code that may suddenly need to rely on this signal, but nothing forces you to do it. And often you'll only realise with difficult to track bugs. This kind of was the entire point of the drop guard: to make it easier to write correct code. But this drop guard impl does not achieve that because it moves some other subtle synchronisation issues to the user.
So really, the only way to make this drop guard work is to really release the mutex in the sync Drop code. Maybe that's doable somehow, but it's so easy.
I looked a bit at the failures now. And while I could fix them I'm a bit unsure how to proceed:
The failures occurring are because async drop is difficult. So this PR works around that by spawning a task when the drop guard is created and the drop guard itself only sends a message to that task to do run the drop impl.
The major problem with this approach is that drop is no longer deterministic. It could run any other time. And this is what makes the tests falky now: sometimes drop has run, sometimes it hasn't and the context is not yet freed.
One could argue this is a matter of more synchronisation: I can add code that sends a signal when drop is finished and we can await until the mutex is truly freed. However this makes using it again brittle: because it is easy enough to write code that may suddenly need to rely on this signal, but nothing forces you to do it. And often you'll only realise with difficult to track bugs. This kind of was the entire point of the drop guard: to make it easier to write correct code. But this drop guard impl does not achieve that because it moves some other subtle synchronisation issues to the user.
So really, the only way to make this drop guard work is to really release the mutex in the sync Drop code. Maybe that's doable somehow, but it's so easy.
Btw, the same problem as i had in accounts.rs:Config::create_lock_task(). I think this can be solved by introducing a Stopping state. So, if it's Stopping, alloc_ongoing() should wait until it stops, but if it's Running, it must fail as now. Btw, u already have ShallStop, maybe it fits?