Switch to using `http::Request` and `http::Response` #286

SebastiaanYN · 2023-02-27T15:24:30Z

This PR switches the library to use http for its request and response types rather than providing its own. This reduces the amount of code needed to maintain and gives access to the wider Rust ecosystem. With this PR it becomes possible to use libraries like axum directly with workers-rs.

The largest changes in this PR include:

Remove worker::Request and worker::Response and use http::Request and http::Response instead.
Provide a Body type that wraps any http_body::Body. All request and response bodies become streaming by default.
Remove the routing logic

There are a couple of points that need to be addressed before merging this PR:

should Body implement .json() and .text() as functions for ease of use? When relying on axum this is not necessary but when writing a worker without an additional layer it would reduce code slightly.
should other library functions, such as R2, return or wrap Body as well to unify interfaces.
all documentation needs to be updated.

An example of calling axum from a worker:

fn router() -> Router<AppState, Body> {
    Router::new()
        .route("/application", post(application))
        .route("/users", get(users))
        .route("/.well-known/jwks.json", get(jwks))
        .nest("/oauth", oauth::router())
}

#[event(fetch)]
async fn fetch(req: Request<Body>, env: Env, _ctx: Context) -> Result<Response<Body>> {
    console_error_panic_hook::set_once();

    let env = Arc::new(env);
    let kv = Arc::new(env.kv("KV").unwrap());

    let res = router()
        .with_state(AppState { env, kv })
        .call(req)
        .await
        .unwrap();

    Ok(res.map(Body::new))
}

spencerbart · 2023-03-03T22:18:47Z

Love this!!

spencerbart · 2023-03-05T21:32:07Z

@SebastiaanYN what can I do to help this move through?

SebastiaanYN · 2023-03-05T21:40:37Z

Providing feedback and pointing out any issues you run into is welcomed. Outside of that there's currently not much to do

jdon · 2023-03-07T01:29:44Z

Hey @SebastiaanYN ,

I've been playing around with the this branch and I've been unable to get WebSocket's to work with durable objects.

I've created a small example that reproduces the issue: https://github.com/jdon/worker-websocket

Running that repo locally using wrangler dev --local, works as expected, I'm able to connect to ws://127.0.0.1:8787/websocket using insomnia and it'll echo back what I send to the server.

After publishing the worker using wrangler publish insomnia fails to connect to the WebSocket at: wss://websocket-worker.jdon.workers.dev/websocket.

When looking at the Real-time Logs in the Cloudflare dashboards any requests to the WebSocket have the status of Canceled. Other requests to the durable object like http://websocket-worker.jdon.workers.dev/add work as expected.
I don't see any errors in the logs for the cancelled requests, so I'm not sure if the issue is with workers-rs or my code.

worker/src/http/request.rs

worker/src/http/response.rs

worker/src/cf.rs

worker/src/body/body.rs

worker/src/router.rs

SebastiaanYN · 2023-03-14T21:35:08Z

@jdon some of the issues were related to response setting a body when it shouldn't. That's now fixed. The other issue is that the request you make to the durable object does not set the upgrade: websocket header as documented on https://developers.cloudflare.com/workers/learning/using-websockets/#writing-a-websocket-client. You can set the headers as following:

stub.fetch_with_request(
    http::Request::get(format!("http://example.com/{path}"))
        .header("upgrade", "websocket")
        .body(().into()),
)
.await;

Thanks for pointing out the issue :)

jdon · 2023-03-14T22:01:03Z

Thanks @SebastiaanYN , I really appreciate you taking the time to look into the issue.

Updating the branch and including the upgrade header fixed it.

SebastiaanYN · 2023-04-11T16:53:13Z

The readme code still has to be updated

nicksrandall · 2023-05-11T13:37:06Z

@SebastiaanYN Do you need any help getting this PR across the finish line? I'd be happy to help.

zebp · 2023-05-31T02:07:47Z

Just to give a bit of a status update on where this is for those wondering, this is still on the radar for a 0.1.x release. There's some further work tangentially related to this PR that needs to be done (ideally) before this gets merged in to make this a smooth transition. That being said if anyone tries this and runs into issues in the meantime please let us know

sam-rusty · 2023-09-24T02:14:23Z

Any update on this PR? eagerly waiting for this to merge for the next version.

JorgeAtPaladin · 2023-11-19T16:09:54Z

Also looking for this to be merged. The fact that we are locked into using the provided "router" is kinda ridiculous

spigaz · 2023-11-19T23:24:34Z

Besides the obvious, that you can always create a fork, merge this change and use the git reference in cargo and off you go. As simple as that...

But actually we are not, there is this approach to run axum for instance
https://logankeenan.com/posts/rust-axum-and-cloudflare-workers/

And in really you don't even have to use workers-rs, or rust, as long as you can compile to wasm and call it from a JS function you can use what you want
https://developers.cloudflare.com/workers/runtime-apis/webassembly/javascript/

JorgeAtPaladin · 2023-11-20T11:34:23Z

@spigaz The mappings look great, thanks! Would love for these to be moved into this repository for maintenance and security reasons.

@kflansburg Can these two functions be moved into the workers-rs repo?

https://github.com/logankeenan/axum-cloudflare-adapter/blob/a475acb54a9f1667c588d7e7a2c928e94afa92ad/adapter/src/lib.rs#L66-L116

kflansburg · 2023-11-20T16:51:44Z

@kflansburg Can these two functions be moved into the workers-rs repo?

https://github.com/logankeenan/axum-cloudflare-adapter/blob/a475acb54a9f1667c588d7e7a2c928e94afa92ad/adapter/src/lib.rs#L66-L116

Those look useful, is it correct to say that they are not needed if we landed this PR instead? I can sync with @zebp on what the remaining blockers are.

zebp · 2023-11-20T17:10:56Z

Those look useful, is it correct to say that they are not needed if we landed this PR instead? I can sync with @zebp on what the remaining blockers are.

Yeah, they wouldn't be needed once this PR gets merged in. Sebastiaan created a doc internally about this PR and some other work he'd like for workers-rs, I'll send you the link.

JorgeAtPaladin · 2023-11-20T21:23:04Z

@zebp @kflansburg , indeed! I think supporting the native request and response is indeed superior. If this is a priority internally I agree the translation functions are not needed to be implemented in this repository.

Thank you all for prioritizing this internally.

avsaase · 2024-03-06T10:03:29Z

I would love to see this merged into the main branch. Is there anything that community members can contribute to get this PR over the finish line? The OP only mentions updating the documentation as an open item.

kflansburg · 2024-03-06T14:40:22Z

I would love to see this merged into the main branch. Is there anything that community members can contribute to get this PR over the finish line? The OP only mentions updating the documentation as an open item.

See #440, there was a bug in streaming bodies that took us a while to track down. What remains is a gap in converting tests over (many are just commented out). If someone could help out with this it would be greatly appreciated.

avsaase · 2024-03-06T20:02:56Z

@kflansburg I went through the PR diff and I don't see any tests that are commented out or that otherwise need to be changed.

kflansburg · 2024-03-06T20:50:54Z

@avsaase Unfortunately I cannot remember the exact place I saw the changes. It's also confusing because #440 is based on a branch that Zeb put together to debug. Anyway, if you look at failing tests for #440 you can see that there are a bunch related to R2 and queues (among others) which I believe never had their routes re-implemented for http. I may be wrong though.

avsaase · 2024-03-06T22:07:39Z

Yeah, for me as an outsider it's a bit hard to make sense of all the branches and follow what has been done and what is still to do. How I understand it now is #440 targets the zeb/http-types branch which looks like a rebase on main of this PR's base branch with one additional commit. Both branches have conflicts with main which for someone with limited knowledge of the library's internals don't seem trivial to resolve.

I'm afraid I can't be of much help with the failing tests in #440 because I'm not familiar with the test harness (and JS and JS tooling for that matter) and I'm not able to reproduce the failing tests locally.

I hope someone else is able to get this PR moving again. Being able to use Axum and it ecosystem (without adapters) would be a huge usability improvement to writing cloudflare workers in Rust.

spigaz · 2024-03-08T23:04:53Z

I'm giving it a look, some are already working, r2, service-bindings and queues, about half I guess.

I had to port some things from the latest version for service-bindings to work, I believe they should be cleared away on rebase.

I'm working on getting streaming to work.

I just posted to avoid duplicate work, I hope to continue tomorrow.

spigaz · 2024-03-10T00:04:24Z

Good news: I only have the D1 tests missing. I expect to continue tomorrow.
https://github.com/spigaz/workers-rs/tree/pr-440

Once reviewed, the rebase...

spigaz · 2024-03-10T20:39:36Z

This means #473 > review > rebase...

sam-rusty · 2024-03-10T21:30:59Z

That is amazing! Great job!!!! <3

avsaase · 2024-03-11T16:50:36Z

Regarding the use of unsafe in this PR, do I understand correctly that this is safe because the worker runtime will always execute a worker on a single thread?

kflansburg · 2024-03-11T17:55:55Z

Regarding the use of unsafe in this PR, do I understand correctly that this is safe because the worker runtime will always execute a worker on a single thread?

Yes

kflansburg · 2024-03-11T19:36:27Z

Closed in favor of #474

SebastiaanYN force-pushed the http branch 2 times, most recently from 272b722 to f91f398 Compare February 27, 2023 16:38

SebastiaanYN force-pushed the http branch from d9420a3 to a6fadf4 Compare March 4, 2023 06:15

zebp reviewed Mar 10, 2023

View reviewed changes

worker/src/http/request.rs Outdated Show resolved Hide resolved

worker/src/http/response.rs Outdated Show resolved Hide resolved

worker/src/cf.rs Outdated Show resolved Hide resolved

worker/src/body/body.rs Outdated Show resolved Hide resolved

worker/src/router.rs Show resolved Hide resolved

SebastiaanYN force-pushed the http branch 4 times, most recently from b6d60e7 to 9a44740 Compare March 14, 2023 21:03

SebastiaanYN force-pushed the http branch 3 times, most recently from 0459bc3 to d710266 Compare March 16, 2023 13:59

SebastiaanYN added 5 commits March 16, 2023 15:06

Switch to using http::Request and http::Response

e212867

Make types and futures Send and Sync

84fd719

Directly get JS body as ArrayBuffer when possible

f7e1a49

Don't set response body if it's empty

0ff037f

Provide clone functions for request and response types

640d906

SebastiaanYN force-pushed the http branch from d710266 to 640d906 Compare March 16, 2023 14:13

SebastiaanYN added 4 commits March 16, 2023 15:44

Make cache API accept both web_sys and http Request/Response

b07ada8

Add request redirect and cf properties back

cb778c5

Add .text() and .json() methods to Body

8595c75

Add documentation

db00947

SebastiaanYN requested a review from zebp April 11, 2023 16:52

KianNH mentioned this pull request Apr 21, 2023

[Feature] Use standardized Rust crate defintions for better interoparability with other Rust crates #307

Open

1 task

sirupsen mentioned this pull request May 23, 2023

Unit testing #174

Open

avsaase mentioned this pull request Mar 10, 2024

fix: Use method of http crate instead of the original method #428

Closed

kflansburg closed this Mar 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using `http::Request` and `http::Response` #286

Switch to using `http::Request` and `http::Response` #286

SebastiaanYN commented Feb 27, 2023 •

edited

Loading

spencerbart commented Mar 3, 2023

spencerbart commented Mar 5, 2023

SebastiaanYN commented Mar 5, 2023

jdon commented Mar 7, 2023 •

edited

Loading

SebastiaanYN commented Mar 14, 2023 •

edited

Loading

jdon commented Mar 14, 2023

SebastiaanYN commented Apr 11, 2023

nicksrandall commented May 11, 2023

zebp commented May 31, 2023

sam-rusty commented Sep 24, 2023

JorgeAtPaladin commented Nov 19, 2023

spigaz commented Nov 19, 2023 •

edited

Loading

JorgeAtPaladin commented Nov 20, 2023

kflansburg commented Nov 20, 2023

zebp commented Nov 20, 2023

JorgeAtPaladin commented Nov 20, 2023

avsaase commented Mar 6, 2024

kflansburg commented Mar 6, 2024

avsaase commented Mar 6, 2024

kflansburg commented Mar 6, 2024

avsaase commented Mar 6, 2024

spigaz commented Mar 8, 2024

spigaz commented Mar 10, 2024

spigaz commented Mar 10, 2024

sam-rusty commented Mar 10, 2024

avsaase commented Mar 11, 2024

kflansburg commented Mar 11, 2024

kflansburg commented Mar 11, 2024

Switch to using http::Request and http::Response #286

Switch to using http::Request and http::Response #286

Conversation

SebastiaanYN commented Feb 27, 2023 • edited Loading

spencerbart commented Mar 3, 2023

spencerbart commented Mar 5, 2023

SebastiaanYN commented Mar 5, 2023

jdon commented Mar 7, 2023 • edited Loading

SebastiaanYN commented Mar 14, 2023 • edited Loading

jdon commented Mar 14, 2023

SebastiaanYN commented Apr 11, 2023

nicksrandall commented May 11, 2023

zebp commented May 31, 2023

sam-rusty commented Sep 24, 2023

JorgeAtPaladin commented Nov 19, 2023

spigaz commented Nov 19, 2023 • edited Loading

JorgeAtPaladin commented Nov 20, 2023

kflansburg commented Nov 20, 2023

zebp commented Nov 20, 2023

JorgeAtPaladin commented Nov 20, 2023

avsaase commented Mar 6, 2024

kflansburg commented Mar 6, 2024

avsaase commented Mar 6, 2024

kflansburg commented Mar 6, 2024

avsaase commented Mar 6, 2024

spigaz commented Mar 8, 2024

spigaz commented Mar 10, 2024

spigaz commented Mar 10, 2024

sam-rusty commented Mar 10, 2024

avsaase commented Mar 11, 2024

kflansburg commented Mar 11, 2024

kflansburg commented Mar 11, 2024

Switch to using `http::Request` and `http::Response` #286

Switch to using `http::Request` and `http::Response` #286

SebastiaanYN commented Feb 27, 2023 •

edited

Loading

jdon commented Mar 7, 2023 •

edited

Loading

SebastiaanYN commented Mar 14, 2023 •

edited

Loading

spigaz commented Nov 19, 2023 •

edited

Loading