Skip to content

make the structs sync + send regardless of features enabled#53

Merged
belchior merged 2 commits into
belchior:mainfrom
norbertkeri:syncsend
May 18, 2025
Merged

make the structs sync + send regardless of features enabled#53
belchior merged 2 commits into
belchior:mainfrom
norbertkeri:syncsend

Conversation

@norbertkeri
Copy link
Copy Markdown

@norbertkeri norbertkeri commented May 14, 2025

Currently, the various select/update/... structs are only sync + send if you don't enable the postgresql or the sqlite features.

These two features add a Vec<dyn WithQuery> property on the structs, which causes the struct to become !Send and !Sync. Apart from the surprise it causes, it also makes it hard to use in web framework handlers, because if any of these structs are held across an .await point, the future will also become !Send, which the web frameworks (like Axum) will reject.

This PR adds the mentioned trait bounds, as far as I can see with no side effects.

@belchior
Copy link
Copy Markdown
Owner

Can you provide a test to this use case? Or a gist with the broken code?

@norbertkeri
Copy link
Copy Markdown
Author

Hmm not sure how to write a test, since the offending code "just" doesn't compile, do you have a suggestion?

The root of the problem is that tokio::spawn requires the given future to be Send. Tokio is used by most (maybe all?) web frameworks, so route handler functions must also be Send. It's a problem in general with anything that uses tokio, not just web frameworks.

A small example:

use std::time::Duration;

async fn task() {
    let builder = sql_query_builder::Select::new();

    tokio::time::sleep(Duration::from_secs(1)).await; // future is not Send as this value is used across an await

    builder.select("*");
}

#[tokio::main]
async fn main() {
    tokio::spawn(async { // future cannot be sent between threads safely ...
        task().await;
    });
}
13  | /     tokio::spawn(async {
14  | |         task().await;
15  | |     });
    | |______^ future created by async block is not `Send`
    |
    = help: the trait `Sync` is not implemented for `(dyn sql_query_builder::behavior::WithQuery + 'static)`
note: future is not `Send` as this value is used across an await
   --> src/main.rs:6:48
    |
4   |     let builder = sql_query_builder::Select::new();
    |         ------- has type `sql_query_builder::Select` which is not `Send`
5   |
6   |     tokio::time::sleep(Duration::from_secs(1)).await;
    |                                                ^^^^^ await occurs here, with `builder` maybe used later
note: required by a bound in `tokio::spawn`

If you don't enable the postgresql or the sqlite feature on this crate, it works fine, since the offending property (dyn WithQuery + 'static) is not present.

@norbertkeri
Copy link
Copy Markdown
Author

Added tests

Comment thread tests/clauses_sync_send.rs Outdated
@belchior belchior added the bug Something isn't working label May 16, 2025
@belchior belchior self-requested a review May 16, 2025 13:02
@norbertkeri
Copy link
Copy Markdown
Author

Done

@belchior belchior merged commit 25bc809 into belchior:main May 18, 2025
@belchior
Copy link
Copy Markdown
Owner

belchior commented May 18, 2025

The version 2.4.2 was published to fix this issue.

Thanks to @knorbert for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants