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

Compile error 'Sync is not implemented' under tokio #86

Closed
tqwewe opened this issue Aug 15, 2021 · 11 comments
Closed

Compile error 'Sync is not implemented' under tokio #86

tqwewe opened this issue Aug 15, 2021 · 11 comments

Comments

@tqwewe
Copy link
Contributor

tqwewe commented Aug 15, 2021

Related issue: SeaQL/sea-query#105

With the following function in a tokio async runtime, I get an error saying:

future created by async block is not `Send`
help: the trait `Sync` is not implemented for `(dyn Iden + 'static)`
async fn update(
    db: &DatabaseConnection,
    active_model: model::ActiveModel,
) -> Result<bool, Box<dyn std::error::Error>> {
    Entity::update(active_model)
        .exec(db)
        .await
        .map_err(Box::new)?;

    Ok(true)
}

SeaORM does not seem to be thread safe. Perhaps this is due to sea-query's Iden type.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 15, 2021

Can you setup an minimal reproducible example under /examples?

@tqwewe
Copy link
Contributor Author

tqwewe commented Aug 15, 2021

I've fixed it locally... the quick fix seems to be changing the definition of trait Iden in sea-query to:

pub trait Iden: Send + Sync {
    ...
}

This is to enforce the type is Sync

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 15, 2021

Great finding. Can we instead do it inside SeaORM, changing IdenStatic instead of Iden?

pub trait IdenStatic: Iden + Copy + Debug + Send + Sync + 'static {

I am trying to understand the problem

@tqwewe
Copy link
Contributor Author

tqwewe commented Aug 15, 2021

I've tried removing Send + Sync from Iden trait in sea-query and adding it to IdenStatic like you suggested, but the errors come back.

T inside Arc<T> should be Send + Sync for the whole Arc to be Send + Sync.
https://doc.rust-lang.org/std/sync/struct.Arc.html#thread-safety
It says:

Arc<T> will implement Send and Sync as long as the T implements Send and Sync. Why can’t you put a non-thread-safe type T in an Arc<T> to make it thread-safe? This may be a bit counter-intuitive at first: after all, isn’t the point of Arc<T> thread safety? The key is this: Arc<T> makes it thread safe to have multiple ownership of the same data, but it doesn’t add thread safety to its data.

Perhaps sea-query should add Send + Sync to the Iden trait only if "thread-safe" flag is enabled.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 15, 2021

Okay, good to know. Thanks.

tyt2y3 added a commit that referenced this issue Aug 15, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Aug 15, 2021

Btw, any idea how can I add a test case to our test suite for this?
Because it worked in our async_std examples.
It might be specific to tokio?

@tqwewe
Copy link
Contributor Author

tqwewe commented Aug 15, 2021

Btw, any idea how can I add a test case to our test suite for this?

Perhaps just making a tokio app and executing your query in a tokio spawned task:

tokio::spawn(async move {
    MyEntity::find().one(&db).await;
})
.await;

It might be specific to tokio?

Perhaps... but I think it might just be any app that uses multiple threads.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 16, 2021

@billy1624 can we also run our test suite using tokio?

@billy1624
Copy link
Member

Planning to do so. Running doctest & unit test on all supported runtime.

@billy1624
Copy link
Member

Planning to do so. Running doctest & unit test on all supported runtime.

See #91

tyt2y3 added a commit that referenced this issue Aug 21, 2021
@tyt2y3 tyt2y3 changed the title Update query not thread safe Compile error: Sync is not implemented under tokio Aug 22, 2021
@tyt2y3 tyt2y3 changed the title Compile error: Sync is not implemented under tokio Compile error 'Sync is not implemented' under tokio Aug 22, 2021
@tqwewe tqwewe mentioned this issue Aug 23, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Aug 23, 2021

I verified that enabling thread-safe on SeaQuery did solve the issue.

@tyt2y3 tyt2y3 closed this as completed Aug 23, 2021
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

No branches or pull requests

3 participants