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

Duplicate test harness using axum router #481

Merged
merged 9 commits into from
Mar 25, 2024
Merged

Conversation

kflansburg
Copy link
Contributor

No description provided.

@kflansburg
Copy link
Contributor Author

Status: trying to get a matching test harness router implemented in axum. This has been a good exercise because there are a lot of sharp edges.

Biggest issue right now is that it seems that any handler which involves a JsFuture (kv::get for instance) doesn't appear to be sync / send as needed.

I know we may not continue using miniflare, but I hope that breaking out the individual test handlers will make converting to wasm bindgen test easier.

@spigaz
Copy link
Contributor

spigaz commented Mar 17, 2024

I know we may not continue using miniflare, but I hope that breaking out the individual test handlers will make converting to wasm bindgen test easier.

@kflansburg My point was to understand your position, not to limit you in any way. Feel free to push forward the best way you can.

Because, there are many things that could be tested using wasm-bindgen-test and for some reason even the existing wasm-bindgen-tests apparently were deprecated.

There are currently only 6 of them, 4 running, 2 broken.

My objective is to run wasm-bindgen-test-runner over miniflare - workerd.

I already started on the wasm-bindgen-test-runner, it seems I'll have to start by writing some tests.

@kflansburg kflansburg changed the title [WIP] Move global fetch to http Duplicate test harness using axum router Mar 21, 2024
@kflansburg
Copy link
Contributor Author

kflansburg commented Mar 21, 2024

Ok, I think this is in pretty good shape. Going to take a docs pass in the morning to explain the utilities for making things Send, but overall I think this turned out pretty well.

Some additional discussion in #485

@kflansburg kflansburg marked this pull request as ready for review March 21, 2024 02:25
Copy link
Contributor

@avsaase avsaase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this a quick try and it seems to work fine with the #[worker::send] macro. I suggest adding a note about it to the readme.

I do think that eventually the better solution is to have Send + Sync wrappers around the wasm_bindgen types but this works well for now.

@dakom
Copy link
Contributor

dakom commented Mar 22, 2024

@kflansburg can you please explain how the send macro is sound?

JsValue types are !Send for good reason afaict. While workers (and wasm in general) is not multithreaded out of the box today, it very well may be in the future.

Let's assume the wasm memory is shared via SharedArrayBuffer - i.e. the Rust side will be genuinely multithreaded, and so something like axum will be able to call its handlers across threads.

Unless I am misunderstanding something, JsValues will still not be thread-safe (e.g. you can't have an instance of a JS class like DurableObject shared between threads, the JS runtime does not allow this and pointers in Rust to these JsValues require maintaining that invariant, I think..)

@kflansburg
Copy link
Contributor Author

kflansburg commented Mar 22, 2024

@kflansburg can you please explain how the send macro is sound?

JsValue types are !Send for good reason afaict. While workers (and wasm in general) is not multithreaded out of the box today, it very well may be in the future.

Let's assume the wasm memory is shared via SharedArrayBuffer - i.e. the Rust side will be genuinely multithreaded, and so something like axum will be able to call its handlers across threads.

Unless I am misunderstanding something, JsValues will still not be thread-safe (e.g. you can't have an instance of a JS class like DurableObject shared between threads, the JS runtime does not allow this and pointers in Rust to these JsValues require maintaining that invariant, I think..)

Everything you say is true, but Workers is single-threaded, and there are no plans for this to change. Marking these types as Send is a pattern that this repo has used for quite a while now.

@dakom
Copy link
Contributor

dakom commented Mar 22, 2024

@kflansburg fair enough! For anyone else who stumbles in this, explanation of why Cloudflare intends to keep Workers always single-threaded: https://developers.cloudflare.com/workers/reference/security-model/#step-1-disallow-timers-and-multi-threading

@kflansburg kflansburg merged commit b7068e2 into main Mar 25, 2024
3 checks passed
@kflansburg kflansburg deleted the kflansburg/http-fetch branch March 25, 2024 16:06
jdon pushed a commit to jdon/workers-rs that referenced this pull request Mar 27, 2024
* Prepare to remove global Fetch for http

* clippy

* Refactor worker test harness

* Axum test harness

* Get more tests working

* Macro for marking future as Send

* Remaining axum routes

* More documentation

* Cleanup
@JorritSalverda
Copy link

JorritSalverda commented Mar 27, 2024

@kflansburg why did you remove pub use crate::schedule::*; in worker/src/lib.rs? After upgrading to v0.0.22 the following no longer works in my scheduled workers:

use worker::{ScheduleContext, ScheduledEvent};

And if it's accident shouldn't there be any tests in the build pipeline to reveal these types of problems? :)

@kflansburg
Copy link
Contributor Author

@kflansburg why did you remove pub use crate::schedule::*; in worker/src/lib.rs? After upgrading to v0.0.22 the following no longer works in my scheduled workers:

use worker::{ScheduleContext, ScheduledEvent};

And if it's accident shouldn't there be any tests in the build pipeline to reveal these types of problems? :)

This was a typo and is fixed here. #501

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.

5 participants