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

Allow async implementation for BackupReader and BackupWriter #1

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

maan2003
Copy link
Member

@maan2003
Copy link
Member Author

removing impls for Read and Write is complicated. I tried but wasn't able to do it. We can reattempt after upstream responds.

Comment on lines +20 to +23
async fn append(&mut self, data: &[u8]) -> std::io::Result<()> {
Write::write_all(self, data)?;
self.flush()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this have to be in some wrapper that tells the async executor that it should be run on a thread that is allowed to block? Otherwise we only fix the problem in case downstream users don't use a writer but implement BackupWriter themselves. Maybe that's the best way to go anyway and we should just remove this default impl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so to avoid bigger changes we can keep it this way if it's only used for testing anyway.

Comment on lines +75 to +79
async fn read(&mut self) -> std::io::Result<Vec<u8>> {
let mut buf = Vec::new();
self.read_to_end(&mut buf)?;
Ok(buf)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this impl is still blocking an async task afaik.

@elsirion elsirion merged commit 8d4df62 into fedimint:main Jan 24, 2024
@elsirion
Copy link

elsirion commented Jan 24, 2024

I'm getting a bunch of clippy warnings, nothing too bad but would be good to fix for the upstream PR.

warning: unused import: `Read`
  --> consensus/src/runway/mod.rs:25:10
   |
25 |     io::{Read, Write},
   |          ^^^^
   |
   = note: `#[warn(unused_imports)]` on by default

warning: unused import: `Read`
  --> consensus/src/member.rs:27:10
   |
27 |     io::{Read, Write},
   |          ^^^^

warning: unused import: `Write`
  --> consensus/src/member.rs:27:16
   |
27 |     io::{Read, Write},
   |                ^^^^^

warning: unused import: `Write`
  --> consensus/src/runway/mod.rs:25:16
   |
25 |     io::{Read, Write},
   |                ^^^^^

warning: `fedimint-aleph-bft` (lib) generated 4 warnings (run `cargo clippy --fix --lib -p fedimint-aleph-bft` to apply 1 suggestion)
    Checking aleph-bft-fuzz v0.0.0 (/home/user/projects/elsirion/fedimint-alephbft/fuzz)
    Checking aleph-bft-examples-blockchain v0.0.3 (/home/user/projects/elsirion/fedimint-alephbft/examples/blockchain)
    Checking aleph-bft-examples-ordering v0.0.3 (/home/user/projects/elsirion/fedimint-alephbft/examples/ordering)
warning: use of `or_insert_with` to construct default value
  --> examples/blockchain/src/data.rs:64:18
   |
64 |                 .or_insert_with(Vec::new)
   |                  ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_or_default
   = note: `#[warn(clippy::unwrap_or_default)]` on by default

warning: use of `or_insert_with` to construct default value
  --> examples/blockchain/src/data.rs:91:14
   |
91 |             .or_insert_with(Vec::new)
   |              ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `or_default()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unwrap_or_default

warning: `aleph-bft-examples-blockchain` (bin "aleph-bft-examples-blockchain") generated 2 warnings (run `cargo clippy --fix --bin "aleph-bft-examples-blockchain"` to apply 2 suggestions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants