Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Call stop_io()/start_io() before/after backup export #2148

Closed
Hocuri opened this issue Mar 1, 2021 · 7 comments · Fixed by #2153
Closed

Call stop_io()/start_io() before/after backup export #2148

Hocuri opened this issue Mar 1, 2021 · 7 comments · Fixed by #2153
Assignees

Comments

@Hocuri
Copy link

Hocuri commented Mar 1, 2021

See deltachat/deltachat-core-rust#2255

For Android, I already made a PR: https://github.com/deltachat/deltachat-android/pull/1822/files

For backup import it's not necessary as IO is not running before doing an import.

For key import/export, it doesn't hurt but isn't necessary.

@Simon-Laux
Copy link
Member

Simon-Laux commented Mar 2, 2021

does this need to go into 1.15 (core 50) or is this specific to the new core?

@Hocuri
Copy link
Author

Hocuri commented Mar 2, 2021

If you only use core releases, then it has to be done before you switch to the next release.

But it has no negative consequences to do it right now, and it's a 2-line-change, so if you ask me, do it now :)

@Hocuri
Copy link
Author

Hocuri commented Mar 2, 2021

Also, by stopping the IO before you do backup export, you already now prevent a possible database corruption (without upgrading the core), yet another reason to do it now 🚀

@Simon-Laux Simon-Laux added new-core involves or requires an dc-node/core update and removed new-core involves or requires an dc-node/core update labels Mar 2, 2021
@Simon-Laux Simon-Laux self-assigned this Mar 2, 2021
@Simon-Laux Simon-Laux added this to To do in Desktop 1.15 via automation Mar 2, 2021
@Simon-Laux
Copy link
Member

ok, doing it now. but question - why is the ui responsible for this and not the core?

Simon-Laux added a commit that referenced this issue Mar 2, 2021
@Simon-Laux Simon-Laux moved this from To do to In progress in Desktop 1.15 Mar 2, 2021
@r10s
Copy link
Member

r10s commented Mar 2, 2021

why is the ui responsible for this and not the core?

yeah, good question, no sure if it was discussed before, so only my 2cents without having followed all discussions to this fix before:

i think, it makes some sense as dc_imex() is also called without IO on initial setup for importing things (IO cannot run there as configuration is not yet available). later, IO is started by the UI.

also, probably the more important point, having functions that implicitly stop IO may easily result in race conditions.

so, even if it cause a few more lines, i think it is a clearer approach to leave start/stop IO completely in the hand of one instance.

but yes, as always, there are many ways to do things :)

@Simon-Laux
Copy link
Member

can we make the backup fail if the io is still running? is this issue/point documented in the documentation? (asking because issues in current ui's won't help new future ui's)

@r10s
Copy link
Member

r10s commented Mar 2, 2021

can we make the backup fail if the io is still running?

this is actually the case, https://github.com/deltachat/deltachat-core-rust/blob/master/src/imex.rs#L689

is this issue/point documented in the documentation?

yip: https://c.delta.chat/classdc__context__t.html#ab04a07bb49363824f6fe3b03e6aaaca7

Desktop 1.15 automation moved this from In progress to Done Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Desktop 1.15
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants