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

Adding Send + Sync trait bounds #13

Closed
wants to merge 12 commits into from

Conversation

raui100
Copy link

@raui100 raui100 commented Dec 5, 2024

Futures that are missing the Send and Sync trait bounds are inconvenient when using a work stealing runtime like tokio.

The following code doesn't compile:

use std::net::SocketAddr;

use clamav_client::tokio::{scan_buffer, Tcp};

#[tokio::main]
async fn main() {
    let tcp = Tcp {
        host_address: "127.0.0.1:3310".parse::<SocketAddr>().unwrap(),
    };
    tokio::spawn(async move { scan_buffer(&[1, 2, 3], tcp, None).await });
}
compiler error
error[E0277]: `dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>` cannot be sent between threads safely
    --> src/bin/tokio.rs:10:5
     |
10   |     tokio::spawn(async move { scan_buffer(&[1, 2, 3], tcp, None).await });
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>` cannot be sent between threads safely
     |
     = help: the trait `Send` is not implemented for `dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>`
     = note: required for `Unique<dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>>` to implement `Send`
note: required because it appears within the type `Box<dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>>`
    --> /Users/raui/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:231:12
     |
231  | pub struct Box<
     |            ^^^
note: required because it appears within the type `Pin<Box<dyn Future<Output = Result<tokio::net::TcpStream, std::io::Error>>>>`
    --> /Users/raui/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/pin.rs:1089:12
     |
1089 | pub struct Pin<Ptr> {
     |            ^^^
note: required because it's used within this `async` fn body
    --> /Users/raui/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clamav-client-1.0.0/src/tokio.rs:248:15
     |
248  |   ) -> IoResult {
     |  _______________^
249  | |     let stream = connection.connect().await?;
250  | |     scan(buffer, chunk_size, stream).await
251  | | }
     | |_^
note: required because it's used within this `async` block
    --> src/bin/tokio.rs:10:18
     |
10   |     tokio::spawn(async move { scan_buffer(&[1, 2, 3], tcp, None).await });
     |                  ^^^^^^^^^^
note: required by a bound in `tokio::spawn`
    --> /Users/raui/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.42.0/src/task/spawn.rs:168:21
     |
166  |     pub fn spawn<F>(future: F) -> JoinHandle<F::Output>
     |            ----- required by a bound in this function
167  |     where
168  |         F: Future + Send + 'static,
     |                     ^^^^ required by this bound in `spawn`
     = note: the full name for the type has been written to '/Users/raui/dev/rs/clamav2/target/debug/deps/tokio-88257f09712d00a7.long-type-314989814868166026.txt'
     = note: consider using `--verbose` to print the full type name to the console

For more information about this error, try `rustc --explain E0277`.
error: could not compile `clamav2` (bin "tokio") due to 1 previous error

This PR adds the corresponding trait bounds, removes the async-trait and upps the version number to 2.0.0 according to semver.

Futures that are missing the Send and Sync trait bounds are inconvenient when a work stealing runtime like tokio
@raui100
Copy link
Author

raui100 commented Dec 5, 2024

Independent from this PR. I've rewritte the library (https://github.com/raui100/rust-clamav-client) to improve the ergonomics:

  • Using the same Tcp and Socket type for sync and async
  • Implemented runtime agnostic async lib which unifies tokio, tokio-stream and async_std

The changes are not backwards compatible and I've changed the API at a few places:
clamav_client::scan_file(file_path, connection, chunk_size) -> connection.scan(file_path, chunk_size)

All tests are adapted and I'm testing everything for the tokio and async_std runtime. The test work on macOS however GitHub Actions fail for ubuntu (and maybe windows).

I wanted to ask if you are interested to adapt all or some of these changes? I'd open a new PR for that. We could also try to minimize the breakage for users by keeping the "old" function based API. I'd prefer to not add yet another ClamAV client to crates.io

@toblux
Copy link
Owner

toblux commented Dec 6, 2024

Thank you! I'll take a closer look in the next few days.

In the meantime, could you add a test for your use case?

Why do you want to bump the version to 2.0.0? It's not a breaking change, is it?

@raui100
Copy link
Author

raui100 commented Dec 6, 2024

I've added a test.

I want to bump to version 2.0.0 because adding a trait bound is a breaking change. Users that have used the library with !Send types won't be able to compile their code anymore.

Also I'd recommend to use the following Socket and Tcp for sync and async code. There is little use in making them generic.

#[derive(Debug, Clone)]
pub struct Socket {
    pub socket_path: std::path::PathBuf,
}

#[derive(Debug, Clone)]
pub struct Tcp {
    pub host_address: std::net::SocketAddr,
}

src/async_std.rs Outdated Show resolved Hide resolved
@raui100
Copy link
Author

raui100 commented Dec 6, 2024

This should be the best of both worlds. There are minimal breaking changes downstream (only the Send trait bound) and the async code works for all async runtimes.

@toblux
Copy link
Owner

toblux commented Dec 6, 2024

I wanted to ask if you are interested to adapt all or some of these changes?

I've been thinking about some of your changes already, but would prefer small, incremental changes instead of a rewrite.

@toblux
Copy link
Owner

toblux commented Dec 6, 2024

I created #14 based on your first commits and simplified it. This should fix your initial Send + Sync problem.

I've also added you to the list of contributors. Thanks again!

I'm open to other changes, but let's keep them small if possible. Otherwise, I won't be able to review them anytime soon.

@toblux toblux mentioned this pull request Dec 7, 2024
@raui100 raui100 closed this Dec 7, 2024
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.

2 participants