[BUG] Rc<RefCell<wasm_bindgen_futures::Inner>> cannot be sent between threads safely

[crestonbunch]

Is there an existing issue for this?

• I have searched the existing issues

What version of workers-rs are you using?

0.0.21

What version of wrangler are you using?

2.13.0

Describe the bug

I see that you have recently added an example with axum. This is great! It would allow use of the entire tower ecosystem.

Env implements unsafe Send which is perfect. But any attempt to use it fails. I cannot figure out how to make this work with any non-trivial example. Could you please share how you imagine this should work? For example, here I have extended the example to try and list R2 objects:

use std::sync::Arc;

use axum::{extract::State, routing::get, Router};
use axum_macros::debug_handler;
use tower_service::Service;
use worker::*;

#[derive(Clone)]
pub struct AppState {
    env: Arc<Env>,
}

fn router(env: Env) -> Router {
    let state = AppState { env: Arc::new(env) };
    Router::new().route("/", get(root)).with_state(state)
}

#[event(fetch)]
async fn fetch(
    req: HttpRequest,
    env: Env,
    _ctx: Context,
) -> Result<axum::http::Response<axum::body::Body>> {
    console_error_panic_hook::set_once();
    Ok(router(env).call(req).await?)
}

#[debug_handler]
pub async fn root(State(AppState { env }): State<AppState>) -> &'static str {
    env.bucket("BUCKET")
        .unwrap()
        .list()
        .execute()
        .await
        .unwrap();
    "Hello Axum!"
}

However, we see this is not possible:

error[E0277]: `Rc<RefCell<wasm_bindgen_futures::Inner>>` cannot be sent between threads safely
   --> src/lib.rs:28:1
    |
28  | #[debug_handler]
    | ^^^^^^^^^^^^^^^^ `Rc<RefCell<wasm_bindgen_futures::Inner>>` cannot be sent between threads safely
29  | pub async fn root(State(AppState { env }): State<AppState>) -> &'static str {
    | --------------------------------------------------------------------------- within this `impl Future<Output = &'static str>`
    |
    = help: within `impl Future<Output = &'static str>`, the trait `Send` is not implemented for `Rc<RefCell<wasm_bindgen_futures::Inner>>`
note: required because it appears within the type `JsFuture`
   --> /Users/creston/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-bindgen-futures-0.4.42/src/lib.rs:101:12
    |
101 | pub struct JsFuture {
    |            ^^^^^^^^
    = note: required because it captures the following types: `worker::ListOptionsBuilder<'_>`, `JsFuture`
note: required because it's used within this `async` fn body
   --> /Users/creston/Projects/workers-rs/worker/src/r2/builder.rs:396:51
    |
396 |       pub async fn execute(self) -> Result<Objects> {
    |  ___________________________________________________^
397 | |         let list_promise = self.edge_bucket.list(
398 | |             js_object! {
399 | |                 "limit" => self.limit,
...   |
420 | |         Ok(Objects { inner })
421 | |     }
    | |_____^
    = note: required because it captures the following types: `axum::extract::State<AppState>`, `Arc<worker::Env>`, `Bucket`, `impl Future<Output = std::result::Result<Objects, worker::Error>>`
note: required because it's used within this `async` fn body
   --> src/lib.rs:29:77
    |
29  |   pub async fn root(State(AppState { env }): State<AppState>) -> &'static str {
    |  _____________________________________________________________________________^
30  | |     env.bucket("BUCKET")
31  | |         .unwrap()
32  | |         .list()
...   |
36  | |     "Hello Axum!"
37  | | }
    | |_^
note: required by a bound in `__axum_macros_check_root_future::check`
   --> src/lib.rs:28:1
    |
28  | #[debug_handler]
    | ^^^^^^^^^^^^^^^^ required by this bound in `check`
    = note: this error originates in the attribute macro `debug_handler` (in Nightly builds, run with -Z macro-backtrace for more info)

Steps To Reproduce

1. Modify axum example with the above code to read R2 bucket
2. Observe failure to build

[kflansburg]

Unfortunately, while Env is Send, r2::Bucket is not. I'm slowly making progress in #481 on improving this, but I'm wondering if there is a better approach then just making everything Send. Ultimately the real problem is that axum think that thinks need to be Sync / Send.

[crestonbunch]

This article and the linked crate do something rather clever: https://logankeenan.com/posts/rust-axum-and-cloudflare-workers/

Axum expects routes to return a Send future, but JS types don't implement Send. This is a problem if you want to use worker::Fetch to make an HTTP request. To work around this issue, I created a macro that wraps the entire Axum route in a wasm_bindgen_futures::spawn_local and passes the result of the route back to the main thread using a oneshot::channel. Credit goes to SebastiaanYN for the solution. I simply created a macro for it. To overcome this issue, you can add the #[worker_route_compat] macro, and it'll make your Axum routes compatible.

It's more of a hack than a solution. I browsed around and there don't seem to be any frameworks that work without futures that are Send. Axum is not unique, I think the ecosystem is built around tokio, etc. which require the Send trait.

[kflansburg]

Ok, great, I’ll check that out. Would be nice to have a generic solution.

[kflansburg]

I think something slightly simpler might be possible, I added a generic macro, worker::send, which does:

SendFuture(async move {
    #fn_body
}).await

where SendFuture is marked as Send

See example here which shouldn't work because ByteStream is not Send:

workers-rs/worker-sandbox/src/fetch.rs

Lines 20 to 39 in fca35f4

	#[worker::send]
	pub async fn handle_fetch_json(
	_req: Request,
	_env: Env,
	_data: SomeSharedData,
	) -> Result<Response> {
	let data: ApiData = Fetch::Url(
	"https://jsonplaceholder.typicode.com/todos/1"
	.parse()
	.unwrap(),
	)
	.send()
	.await?
	.json()
	.await?;
	Response::ok(format!(
	"API Returned user: {} with title: {} and completed: {}",
	data.user_id, data.title, data.completed
	))
	}

[kflansburg]

@avsaase I think this might be a cleaner direction for us to go to fix the remaining !Send issues ^

[avsaase]

Marking things Send that are fundamentally not Send feels wrong but I still think it's the best way forward. If I understand correctly, the main issue is that JS types from wasm_bindgen are !Send. The original PR for the most part addressed this by creating Send wrappers around this types. Would that be an option?

My problem with the #[wasm_compat] macro of the axum-cloudflare-adapter is that RA fails to generate semantic highlighting info for VSCode when annotating my handlers. I haven't checked if that happens here as well.

[avsaase]

There are more places where axum requires Send/Sync, most notably the state that you attach to the router.

[kflansburg]

The original PR for the most part addressed this by creating Send wrappers around this types. Would that be an option?

I'm just a little worried about having to go through wrap every single type. It feels time consuming and error prone. This addresses the issue right at the handler level.

My problem with the #[wasm_compat] macro of the axum-cloudflare-adapter is that RA fails to generate semantic highlighting info for VSCode when annotating my handlers. I haven't checked if that happens here as well.

I haven't run into this issue. What does not get highlighted?

There are more places where axum requires Send/Sync, most notably the state that you attach to the router.

I think for this we have Send implemented for Env and Context so that should be covered.

[avsaase]

I'm just a little worried about having to go through wrap every single type. It feels time consuming and error prone. This addresses the issue right at the handler level.

I think adding this won't be too much work. I'm happy to give it a try.

I haven't run into this issue. What does not get highlighted?

Iirc it failed to highlight variable and function names. I'll give it another try with the proposed new macro and report back.

I think for this we have Send implemented for Env and Context so that should be covered.

We have type safe wrappers around other worker types like KvStore and Queue. If these are not Send/Sync it would block us from using the axum router. I think it makes sense to implement Send/Sync for the simplest types and let the compiler infer the "thread safety" for all the types that wrap them. This gives the most flexibility to the SDK user.

[kflansburg]

I'm just a little worried about having to go through wrap every single type. It feels time consuming and error prone. This addresses the issue right at the handler level.

I think adding this won't be too much work. I'm happy to give it a try.

I just worry that this will be hard to maintain, it also doesn't help the user in the case where they implement their own JS bindings.

There are also other cases like: a430c23#diff-958be99384265b0bc1095abf1c900139f0b49da235617bdd1d080dc510ba731eR36

We manipulate JS types in a lot of our method implementations and have to re-factor them to avoid holding them across await boundaries.

[avsaase]

Hmm, I see how creating Send and Sync wrappers for the JS types can become quite complex.

For advanced use cases like ours I think it's not too uncommon to want to use bindings like Queue and KvStore as state. For that to work they need to be Send/Sync regardless of whether the JS types implement those traits (I initially thought they were wrappers around JS types but that;s not the case). Unfortunately the macro you created won't solve this issue so I hope I can convince you to add the Send/Sync impls for the remaining binding types.

[kflansburg]

Hmm, I see how creating Send and Sync wrappers for the JS types can become quite complex.

For advanced use cases like ours I think it's not too uncommon to want to use bindings like Queue and KvStore as state. For that to work they need to be Send/Sync regardless of whether the JS types implement those traits (I initially thought they were wrappers around JS types but that;s not the case). Unfortunately the macro you created won't solve this issue so I hope I can convince you to add the Send/Sync impls for the remaining binding types.

Would it not make sense to pass in Env as state, and then fetch KvStore and Queue within the context of the handler? When would you want to attach these types as router state?

[avsaase]

I tried to get the example in the OP working by implementing Send/Sync for all the R2 types (63cb306) but it still complains about Rc<RefCell<wasm_bindgen_futures::Inner>> not being thread safe. It looks like this is also going to be a problem with awaiting responses from a KvStore (and likely other bindings). The macro solves this issue but not the state trait bound.

Would it not make sense to pass in Env as state, and then fetch KvStore and Queue within the context of the handler? When would you want to attach these types as router state?

We have a worker with 100+ routes that uses a lot of kv stores and queues. To make this type safe we defined types like this:

pub struct KvStoreClient<T> {
    kv_store: KvStore,
    _marker: std::marker::PhantomData<T>,
}

with methods to perform operations in a type safe manner. All this is initialized at the top of main, put into a struct and attached to the worker router with with_data. I would like to keep the initialization logic out of the individual handlers.

[avsaase]

Maybe the macro is the best solution after all. We'll have to unsafe implement Send and Sync for our app state (which I was hoping to avoid) but I think there's no way around that without a large refactor of the worker crate to use SendFuture the crate.

[kflansburg]

I see, well I'm not opposed to implementing Sync / Send for specific types like Env, Context, and others (some of these changes have already landed), but in general I don't want to have to do this for every type a user could possibly access from the worker crate, that makes me even more comfortable given the hack we are using.

I think your issue with awaiting responses from KvStore is exactly what I'm talking about, this involves a Future that isn't Send. So I think you can do something like:

// the scope here is sometimes necessary to make sure everything but the `SendFuture` is dropped before the `await`
let fut = {
    worker::SendFuture::new(env.bucket("BUCKET")
        .unwrap()
        .list()
        .execute())
};


let objects = fut.await.unwrap()

Edit: or the whole handler can be made send using the macro I'm suggesting.

[kflansburg]

Maybe the macro is the best solution after all. We'll have to unsafe implement Send and Sync for our app state (which I was hoping to avoid) but I think there's no way around that without a large refactor of the worker crate to use SendFuture the crate.

I wonder if we could provide something like SendFuture that makes an arbitrary type Send and implements Deref for that type. Maybe that could make this slightly less annoying for the user?

[avsaase]

After thinking about this some more this I think there are two ways to go about this that will yield a good developer experience:

1. Push the unsafe impl Send/Sync's all the way down to the types that come from wasm_bindgen. You could even do this in the worker-sys crate (and add a big fat warning not to use it in a multi-threaded context) or in the worker wrappers like we've been doing so far. This has the benefit that everything else is automatically taken care off by the compiler. After the initial work (I don't know how much that is) I reckon that the maintenance burden of this approach is minimal.

2. Go in the opposite direction and solve the problem at the handler level with the #[worker::send] macro. This requires the user to do their own unsafe impl Send/Sync for their state if if contains other worker types, which might scare off some users.

Note that it's possible to go with the second approach first and later implement the first without it being a breaking change.

I wonder if we could provide something like SendFuture that makes an arbitrary type Send and implements Deref for that type. Maybe that could make this slightly less annoying for the user?

I'm not exactly sure what you have in mind here but I think it falls somewhere in between these two extremes? I worry that it would require the user to add extra steps to their code that aren't really necessary if we pick one of the other two options.

In any case, I think the current selection of types that satisfy the Send + Sync bound is somewhat arbitrary and that an all-or-nothing approach is best.

[spigaz]

To be honest, I don't want to barge in, because you are having a very deep discussion on this already, and I simply don't know the best approach.

But I believe you have to take in consideration that the bugs aren't always obvious and might seem even not related.

A good example was a bug from @avsaase that was blocking him and he had no idea that worker::send was the fix, once I was able to spot with a helper macro

Rc<RefCell<wasm_bindgen_futures::Inner>>` cannot be sent between threads safely

Things got a lot better, and send fixed it, but it was giving errors regarding traits implementations.

Even with a notice, this might impose a hard challenge to anyone landing on workers-rs from other places...

[kflansburg]

Agree that these error messages aren't great. What does the error message look like if axum's debug macro is used? I think they may already have done some work here.

https://docs.rs/axum-macros/latest/axum_macros/attr.debug_handler.html

[avsaase]

Here's the example I created when I was trying to debug a trait bound error on a middleware:

use axum::{
    body::Body,
    extract::{Request, State},
    http::HeaderMap,
    middleware::{from_fn_with_state, Next},
    response::Response,
    routing::get,
};
use axum_cloudflare_adapter::{to_axum_request, to_worker_response};
use tower_service::Service;

#[worker::event(fetch)]
pub async fn main(
    req: worker::Request,
    env: worker::Env,
    _ctx: worker::Context,
) -> worker::Result<worker::Response> {
    let state = AppState {
        kv: env.kv("kv_binding").unwrap(),
    };

    let axum_request = to_axum_request(req).await.unwrap();
    let axum_response = axum::Router::new()
        .route("/v0/test", get(|| async { "Hello, World!" }))
        .layer(from_fn_with_state(
            state.clone(),
            move |state, req: Request, next| my_middleware(state, req.headers().clone(), req, next),
        ))
        .with_state(state)
        .call(axum_request)
        .await?;
    let worker_response = to_worker_response(axum_response).await.unwrap();
    Ok(worker_response)
}

#[derive(Clone)]
struct AppState {
    kv: worker::kv::KvStore,
}

unsafe impl Send for AppState {}
unsafe impl Sync for AppState {}

// #[axum_macros::debug_handler]
// #[worker::send]
async fn my_middleware(
    State(state): State<AppState>,
    _headers: HeaderMap,
    request: Request,
    next: Next,
) -> Response<Body> {
    let value = state.kv.get("key").text().await.unwrap();
    next.run(request).await
}

With the #[axum_macros::debug_handler] macro I get this error:

error[E0277]: `*mut u8` cannot be sent between threads safely
   --> src/lib.rs:44:1
    |
44  |   #[axum_macros::debug_handler]
    |   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut u8` cannot be sent between threads safely
45  |   // #[worker::send]
46  | / async fn my_middleware(
47  | |     State(state): State<AppState>,
48  | |     _headers: HeaderMap,
49  | |     request: Request,
50  | |     next: Next,
51  | | ) -> Response<Body> {
    | |___________________- within this `impl Future<Output = axum::http::Response<axum::body::Body>>`
    |
    = help: within `impl Future<Output = axum::http::Response<axum::body::Body>>`, the trait `Send` is not implemented for `*mut u8`, which is required by `impl Future<Output = axum::http::Response<axum::body::Body>>: Send`
note: required because it appears within the type `PhantomData<*mut u8>`
   --> /home/alexander/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/marker.rs:811:12
    |
811 | pub struct PhantomData<T: ?Sized>;
    |            ^^^^^^^^^^^
note: required because it appears within the type `JsValue`
   --> /home/alexander/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-bindgen-0.2.92/src/lib.rs:91:12
    |
91  | pub struct JsValue {
    |            ^^^^^^^
note: required because it appears within the type `worker::worker_kv::GetOptionsBuilder`
   --> /home/alexander/.cargo/registry/src/index.crates.io-6f17d22bba15001f/worker-kv-0.6.0/src/builder.rs:111:12
    |
111 | pub struct GetOptionsBuilder {
    |            ^^^^^^^^^^^^^^^^^
note: required because it's used within this `async` fn body
   --> /home/alexander/.cargo/registry/src/index.crates.io-6f17d22bba15001f/worker-kv-0.6.0/src/builder.rs:154:64
    |
154 |       pub async fn text(self) -> Result<Option<String>, KvError> {
    |  ________________________________________________________________^
155 | |         let value = self.value_type(GetValueType::Text).get().await?;
156 | |         Ok(value.as_string())
157 | |     }
    | |_____^
    = note: required because it captures the following types: `axum::extract::State<AppState>`, `AppState`, `HeaderMap`, `axum::http::Request<axum::body::Body>`, `Next`, `Option<std::string::String>`, `impl Future<Output = Result<Option<std::string::String>, KvError>>`, `impl Future<Output = axum::http::Response<axum::body::Body>>`
note: required because it's used within this `async` fn body
   --> src/lib.rs:51:21
    |
51  |   ) -> Response<Body> {
    |  _____________________^
52  | |     let value = state.kv.get("key").text().await.unwrap();
53  | |     next.run(request).await
54  | | }
    | |_^
note: required by a bound in `__axum_macros_check_my_middleware_future::check`
   --> src/lib.rs:44:1
    |
44  | #[axum_macros::debug_handler]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `check`
    = note: this error originates in the attribute macro `axum_macros::debug_handler` (in Nightly builds, run with -Z macro-backtrace for more info)