axum icon indicating copy to clipboard operation
axum copied to clipboard

Add tokio feature and make tokio optional

Open fisherdarling opened this issue 2 years ago • 13 comments

Motivation

I'm trying to use axum's routing and request extraction hardware for a Cloudflare Worker. Workers run in a WASM context (v8 isolate) and therefore tokio cannot be used.

Solution

This PR adds a tokio feature that enables tokio as well as hyper/tcp and hyper/server. The tokio feature also enables SSE and extract::connect_info.

This is a breaking change since this breaks usages of default_features = false.

fisherdarling avatar May 25 '22 11:05 fisherdarling

Not quite sure how but maybe we should add a step to CI that somehow verifies that tokio isn't pulled in when this feature isn't enabled. Otherwise I imagine its the kind of thing we could easily break by accident.

The feature should also be mentioned here.

davidpdrsn avatar May 25 '22 11:05 davidpdrsn

Not quite sure how but maybe we should add a step to CI that somehow verifies that tokio isn't pulled in when this feature isn't enabled. Otherwise I imagine its the kind of thing we could easily break by accident.

The feature should also be mentioned here.

That's a really good idea. Maybe a combination of cargo metadata / cargo tree and grepping for tokio might suffice. The only way to truly remove tokio is make it optional for hyper, since hyper always pulls in tokio. I'm looking into that now.

And I'll add the feature to the docs.

Are we good with naming it server?

fisherdarling avatar May 25 '22 11:05 fisherdarling

That's a really good idea. Maybe a combination of cargo metadata / cargo tree and grepping for tokio might suffice. The only way to truly remove tokio is make it optional for hyper, since hyper always pulls in tokio. I'm looking into that now.

Yeah something like that could probably work.

Are we good with naming it server?

How about just calling the feature tokio? Maybe thats more clear 🤔

davidpdrsn avatar May 25 '22 11:05 davidpdrsn

@fisherdarling whats the status of this? I remember we talked a bunch in Discord but don't remember if we reached any conclusions.

davidpdrsn avatar May 27 '22 10:05 davidpdrsn

@davidpdrsn I'm currently blocked on trying to figure out CI. cargo check -p axum --no-default-features --target wasm32-unknown-unknown is failing for me, even though I can successfully use this fork to compile and run a router in a wasm context. I think dead-code elimination is what's allowing me to compile axum as a dependency with --target wasm32-unknown-unknown.

Maybe an example needs to be created that uses axum just for its router, so that mio won't be compiled? Checking that example with the wasm target might work as a CI check.

I'm going to be traveling the next two weeks, but I might have time tonight or this weekend to finish it up.

fisherdarling avatar May 27 '22 12:05 fisherdarling

@fisherdarling You mentioned on discord that you had run into issues with having to run !Send futures and how that might be a show stopper for this. What do you think? Should we close this while you try and get it working? I'm a bit reluctant to merge this if the use case we had in mind doesn't actually work.

davidpdrsn avatar Jun 11 '22 19:06 davidpdrsn

@davidpdrsn I was actually able to get it to work! I've actually switched almost all of my worker code to use the router, which has been extremely nice to have. I just needed a MakeSendFuture like you suggested in the discord.

Axum regularly has breaking changes, so I think removing the tokio feature in the future if nobody uses it would be fine / acceptable. The changes themselves are pretty minimal and don't really infect the code-base, so I think development overhead of having a tokio feature is not too high?

Honestly, I'm happy to just maintain my own fork for now if you don't want to merge it in or are hesitant to do so.

The "true" fix would be to separate the router into an axum-router crate and let people just pull that in.

fisherdarling avatar Jun 12 '22 11:06 fisherdarling

So do you need the Router for your wasm use case or not? If not, should that be the unit that's made optional maybe?

jplatte avatar Jun 12 '22 14:06 jplatte

So do you need the Router for your wasm use case or not? If not, should that be the unit that's made optional maybe?

@jplatte I do need the Router for my wasm use case and this PR does allow me to use the Router in a wasm context.

fisherdarling avatar Jun 12 '22 17:06 fisherdarling

Then what did you mean by

The "true" fix would be to separate the router into an axum-router crate and let people just pull that in.

jplatte avatar Jun 12 '22 21:06 jplatte

Then what did you mean by

The "true" fix would be to separate the router into an axum-router crate and let people just pull that in.

Sorry, that was definitely a little confusing and needs some context.

Just to be clear, my overall goal is to use anaxum::Router for a wasm32-unknown-unknown target.

Everything that axum provides that requires an underlying runtime or server is not necessary for my use case and generally leads to compilation issues for wasm targets. That's why this PR adds a feature flag for that code.

All of the code that I do want, is all of the awesome code that makes it easy to work with http::{Request, Response}. That includes the Router, the great IntoResponse impls, and the extractors that deal with just the request itself (minus things like connection info). This PR allows me to turn off the tokio parts while still being able to use the features I want.

What I meant by this:

The "true" fix would be to separate the router into an axum-router crate and let people just pull that in.

Is that I think it would be beneficial to have the axum crate's router/extractors be separated into their own crates:

  • axum-extractors would be all of the "builtin" extractors (essentially everything under axum/src/extract). Extractors that require a runtime, or rather, extractors that may not be needed in every use case, should be feature flagged. For example, the json extractor (which already is behind a flag) and the ConnectInfo extractor.
  • axum-router would provide the actual router type and the interactions with tower and http. This would be essentially everything in axum except for the extractors. Anything involving tokio directly would be behind a non-default feature flag. This code could (?) also live under axum-core.
  • axum would then mainly just be re-exports of axum-extractors and axum-router/core along with any helper utilities. It would have essentially the same API as it does now.

With those crates split out, I could just pull in axum and the extractors I want behind a feature flag, or I could stay "lightweight" and pull in axum-router and the extractors I want from axum-extractors. The latter option would hopefully help with code size. I could even pull in just axum-router and then implement the extractors I want by hand.

I think this is something that would be beneficial, but I also think that the number of people that would directly benefit from this is rather low (just me right now). Most people (everyone so far?) are running axum on a server with tokio and don't care about nor need fine-grained crate imports.

Overall, the current PR lets me use the parts of the crate I love for a wasm target. Sorry if this is kind of a ramble.

fisherdarling avatar Jun 13 '22 09:06 fisherdarling

The idea of putting the router in its own crate has come up before but I'm a bit hesitant towards that. I'm worried we'll end up with too many axum-* crates that'll be hard for people to understand. For context tower used to be split into separate crates but was merged into one with feature flags because users complained.

So I'm in favor of keeping Router in axum and then adding features. Basically what this PR does.

I'm gonna try and take a look at this this week but since its a breaking change it wont be released for a while. We don't have any concrete plans for 0.6 and there are some things in the pipeline I wanna work on first (such as matchit/routing changes)

davidpdrsn avatar Jun 13 '22 11:06 davidpdrsn

I completely agree and understand. I remember when tokio had so many tokio_* crates and how that was confusing for many people.

I'm gonna try and take a look at this this week but since its a breaking change it wont be released for a while. We don't have any concrete plans for 0.6 and there are some things in the pipeline I wanna work on first (such as matchit/routing changes)

Sounds good, I'll try to keep the branch updated to main.

fisherdarling avatar Jun 13 '22 12:06 fisherdarling

@fisherdarling

I think this is something that would be beneficial, but I also think that the number of people that would directly benefit from this is rather low (just me right now).

I'm very interested in this and seeing this PR made my day! I was able to get a web app running in a CF worker using a fork of Tide. However, my PR for WASM support is stagnant. I'd be interested in helping out in any way to help move this PR along.

I was also able to "run" the Tide web app as an SPA, I imagine this PR would allow for the same. I'm going to pull this PR down and give it a try when I have time this weekend.

logankeenan avatar Aug 26 '22 03:08 logankeenan

This PR works well running in the browser. I made a simple example repo with a demo.

@davidpdrsn - thanks for your help on Discord!

logankeenan avatar Sep 01 '22 02:09 logankeenan

On the other hand, this PR doesn't build for me. As you mentioned above, it still pulls tokio because of tower and hyper, as shown in this reverse dependency tree:

tokio v1.21.0
├── hyper v0.14.20
│   └── axum v0.5.7 (https://github.com/fisherdarling/axum?branch=feat/optional-tokio#ff545b78)
│       └── (my crate)
└── tower v0.4.13
    ├── axum v0.5.7 (https://github.com/fisherdarling/axum?branch=feat/optional-tokio#ff545b78) (*)
    └── tower-http v0.3.4
        └── axum v0.5.7 (https://github.com/fisherdarling/axum?branch=feat/optional-tokio#ff545b78) (*)

Which wouldn't be horrible, because some tokio features do work on wasm32-unknown-unknown. The build fails for me, because axum uses tower's make feature, which in turn uses tokio's std-io feature. It's possible this feature is not really necessary.

edit: Removing the "make" feature in Cargo.toml (l. 44) makes it build 🎉 It seems to be used in 1 test utility, so don't just remove that in this PR

kpietraszko avatar Sep 11 '22 17:09 kpietraszko

Hey all, sorry for being MIA! I just started a new job and have been pretty busy these past couple of months.

@logankeenan I'm glad the PR worked for you and that it benefits you! I would love some help moving this PR along if you can. I won't have the time to work on it for a while. I think next steps are to evaluate if the wasm-wasi change can be used and is helpful. If you want to start from scratch in another PR feel free to. I'll close this one and link to the new one.

@kpietraszko It has to pull in tokio right now because of AsyncRead and AsyncWrite. Super annoying but until that's split out into its own crate we're stuck.

fisherdarling avatar Sep 14 '22 16:09 fisherdarling

@logankeenan feel free to ping me @fisher in the axum Discord if you'd like :)

fisherdarling avatar Sep 14 '22 16:09 fisherdarling

@fisherdarling WASI support is a great addition, but the use-case is a bit different. For example Cloudflare Workers don't support WASI modules out of the box, only WASM. One would need to create their own JS shim I believe.

That said there are some services that allow hosting WASI, such as Fastly.

kpietraszko avatar Sep 14 '22 19:09 kpietraszko

@fisherdarling

I haven't worked with WASI before, but that doesn't mean I couldn't learn more about it. Could that work potentially be done in another PR?

PS, congratulations on the new job!

logankeenan avatar Sep 15 '22 01:09 logankeenan

@fisherdarling - I created a new PR based on your branch.

cc @kpietraszko

logankeenan avatar Sep 17 '22 13:09 logankeenan

Let's continue the in https://github.com/tokio-rs/axum/pull/1382

@fisherdarling thanks for your work on this.

davidpdrsn avatar Sep 18 '22 17:09 davidpdrsn