Allow async implementation for BackupReader and BackupWriter

[maan2003]

using io::Write or io::Read forces a sync implementation.

we were using block_on to use async inside the implementation, but this blocking the executor and sometimes leads to deadlocks.

this is backwards compatible by implementing new traits for any io::Read and io::Write impls.

see fedimint/fedimint#4054 for more context.

[github-actions]

Please make sure the following happened

• Appropriate tests created
• Infrastructure updated accordingly
• Updated existing documentation
• New documentation created
• Version bumped if breaking changes

[dpc]

this is backwards compatible by implementing new traits for any io::Read and io::Write impls.

Isn't the previous way strictly incorrect? The code running it is async, so it can't be calling blocking IO methods without some form of offloading from the current executor (like tokio::task::block_in_place). The current backward-compat impls preserve the existing incorrect behavior.

[maan2003]

should we just remove Read and Write impls. we can't use offloading methods because we don't depend on tokio.

[dpc]

should we just remove Read and Write impls. we can't use offloading methods because we don't depend on tokio.

IMO there's no reason to try to preserve the old (incorrect) behavior.

[maan2003]

we can't remove Read and Write because mock crate impls Read for mock reader.

mock can't impl BackupReader because mock can't depend main crate (circular deps).
Solution to this would be moving mock to same crate, but that will be big change.

[dpc]

mock can't impl BackupReader because mock can't depend main crate (circular deps).

So a mock can't depend on the thing it's mocking? Weird, but I don't have time to dig into it.

Solution to this would be moving mock to same crate, but that will be big change.

Or move the trait to a separate crate, so both can depend on it. That's how Read was working - it was in a 3rd crate (stdlib).

[joschisan]

we can't remove Read and Write because mock crate impls Read for mock reader.

mock can't impl BackupReader because mock can't depend main crate (circular deps). Solution to this would be moving mock to same crate, but that will be big change.

I think we can move the new traits into the aleph-bft-types crate, that's were the other traits used in the api like Network are too.

[woocash2]

Hi, we have looked into your PR and into the issues in your repository, apologies for letting it hang here for a long time. We have introduced traits Read and Write here mostly with an intention of using io::File or a similar kind of a lightweight object with read/write semantics as the implementation. Although we consider it as the preferred way to implement the backup, we understand your need for an asynchronous interface and we're eager to merge the PR in some form. I had my colleague leave some comments so please act upon them, also we'll have to run some checks. Thank you for your contribution.

[dpc]

We have introduced traits Read and Write here mostly with an intention of using io::File

If it was to be done this way then the core alephbft async code calling this interface should be calling it wrapped in some https://docs.rs/tokio/latest/tokio/task/fn.block_in_place.html , otherwise async executor gets blocked on blocking IO operations, potentially stalling async executor.

[maan2003]

• switched to Async{Read,Write}
• rebased on master
• version bump to 0.34

[timorleph]

Nice, thanks! One little comment left.

[timorleph]

Oh, and some failing checks, mostly related to version bumps, one of which should happen in the readme.

[timorleph]

mock and rmc still need version bumps, minor and patch respectively. Although tbh I'm not quite sure why you touched rmc at all?

[maan2003]

maybe version checks don't understand that removing imports doesn't need bump?

[maan2003]

oh those were pub use but not exported at top level