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

Using the format!() macro with an async function makes the whole Future non-Send #101650

Open
Hocuri opened this issue Sep 10, 2022 · 3 comments
Open
Labels
A-fmt Area: std::fmt C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Hocuri
Copy link

Hocuri commented Sep 10, 2022

I tried this code: (minimal reproducible example; for the actual code see deltachat/deltachat-core-rust#3591)

use core::future::Future;
use core::pin::Pin;

fn build_string() -> Pin<Box<dyn Future<Output = String> + Send>> {
    Box::pin(async move {
        let mut string_builder = "<h>".to_string();
        string_builder += &format!("Hello {}", helper().await);
        string_builder += "</h>";
        string_builder
    })
}

async fn helper() -> String {
    "World".to_string()
}

Playground

Note that we are using,

format!("Hello {}", helper().await)

in a context where the Future needs to be Send; only this line without the string_builder seems not to be able to reproduce the issue though.

I expected to see this happen: That the code behaves the same as

let s = helper().await;
format!("Hello {}", s)

Instead, this happened:

There are two error messages (about line 5 and line 7); this issue is about the first one:

error: future cannot be sent between threads safely
 --> src/lib.rs:5:5
  |
5 |     Box::pin(async move {
  |     ^^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
  |
  = help: the trait `Sync` is not implemented for `core::fmt::Opaque`
note: future is not `Send` as this value is used across an await
 --> src/lib.rs:7:56
  |
7 |         string_builder += &format!("Hello {}", helper().await);
  |                            ----------------------------^^^^^^-
  |                            |                   |       |
  |                            |                   |       await occurs here, with `helper().await` maybe used later
  |                            |                   has type `ArgumentV1<'_>` which is not `Send`
  |                            `helper().await` is later dropped here
  = note: required for the cast to the object type `dyn Future<Output = String> + Send`

The second error message didn't appear in the original issue in deltachat/deltachat-core-rust#3591, possibly because there the call to Box::pin() and the format!() call are in two different crates:

error: generator cannot be sent between threads safely
  --> deltachat-jsonrpc/src/api/mod.rs:74:1
   |
74 | #[rpc(all_positional, ts_outdir = "typescript/generated")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
   |
   = help: the trait `Sync` is not implemented for `core::fmt::Opaque`
   = note: required for the cast to the object type `dyn Future<Output = Result<Value, yerpc::Error>> + Send`
   = note: this error originates in the attribute macro `rpc` (in Nightly builds, run with -Z macro-backtrace for more info)

Meta

rustc --version --verbose

Probably doesn't matter, anyway:

rustc 1.61.0 (fe5b13d68 2022-05-18)
binary: rustc
commit-hash: fe5b13d681f25ee6474be29d748c65adcd91f69e
commit-date: 2022-05-18
host: x86_64-unknown-linux-gnu
release: 1.61.0
LLVM version: 14.0.0

@rustbot label +A-fmt

@Hocuri Hocuri added the C-bug Category: This is a bug. label Sep 10, 2022
@rustbot rustbot added the A-fmt Area: std::fmt label Sep 10, 2022
@Hocuri
Copy link
Author

Hocuri commented Sep 10, 2022

Notes:

I'll guess this is
@rustbot label +T-libs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Sep 10, 2022
Jikstra added a commit to deltachat/deltachat-core-rust that referenced this issue Sep 10, 2022
Fix get_connectivity_html and get_encrinfo futures not being Send. See rust-lang/rust#101650 for more information.

Co-authored-by: jikstra <jikstra@disroot.org>
Simon-Laux added a commit to deltachat/deltachat-core-rust that referenced this issue Sep 11, 2022
Fix get_connectivity_html and get_encrinfo futures not being Send. See rust-lang/rust#101650 for more information.

Co-authored-by: jikstra <jikstra@disroot.org>
Simon-Laux added a commit to deltachat/deltachat-core-rust that referenced this issue Sep 11, 2022
* add more functions, see changelog for details

* add pr number to changelog

* clarify doc comment

* clarify usage of BasicChat
and adjust properties acordingly

r10s is right it should only contain what we need of the expensive calls

* fix doc typos

* run cargo fmt

* jsonrpc: add connectivity functions

* fix typo

* fix typo

* Add get_contact_encryption_info and get_connectivity_html

Fix get_connectivity_html and get_encrinfo futures not being Send. See rust-lang/rust#101650 for more information.

Co-authored-by: jikstra <jikstra@disroot.org>

* Update CHANGELOG

* Update typescript files

* remove todo from changelog

Co-authored-by: jikstra <jikstra@disroot.org>
@bjorn3
Copy link
Member

bjorn3 commented Sep 13, 2022

Maybe this can be fixed with #99012

Maybe, maybe not. At the very least I think we should prevent this from accidentally starting to work as that would prevent ever returning to the old code even if the new code is found to have severe issues.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 18, 2022

This isn't a "bug", in the sense that this behaviour is as expected. However, that doesn't mean that we shouldn't consider changing the behaviour.

The problem is that format!("{}{}", a, b) currently erases the types of a and b, but we don't want to accidentally keep the non-Send a around if b is an .await expression. So the type-erased argument (ArgumentV1) is simply never Send, since it's not generic over the type it wraps.

#99012 could indeed fix it, but as @bjorn3 mentions, we should be careful not to accidentally make something new work without first considering whether we want to commit to the new behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fmt Area: std::fmt C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants