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

Clone state a bit less #1837

Merged
merged 5 commits into from
Mar 12, 2023
Merged

Clone state a bit less #1837

merged 5 commits into from
Mar 12, 2023

Conversation

davidpdrsn
Copy link
Member

This reduces the state clones by 1 for each request. Not much but better than nothing. Had to inline the call_route to get around some borrowing issues.

The remaining clones are

  1. https://github.com/tokio-rs/axum/blob/main/axum/src/handler/service.rs#L171
    • I'm gonna invest making Handler::call take S by reference rather than value. Not sure what the implications are. Think the response future should still be 'static so might not be possible.
  2. https://github.com/tokio-rs/axum/blob/main/axum-core/src/extract/from_ref.rs#L23
    • This one we can't do much about. Its the State extractor via FromRef. That needs to produce an owned state.
  3. https://github.com/tokio-rs/axum/blob/main/axum/src/routing/route.rs#L48
    • Because of backpressure Route::call uses oneshot which requires clone. Not sure its possible to fix but worth investigating.
  4. https://github.com/tokio-rs/axum/blob/main/axum/src/test_helpers/test_client.rs#L32 (when hyper calls call on Shared that clones the service)
    • Nothing we can do about this. Hyper needs an owned service to serve the connection.

Part of #1827

axum/src/routing/tests/mod.rs Show resolved Hide resolved
@davidpdrsn
Copy link
Member Author

Ah okay I messed up the tests a bit. I'll fix that later 😅

@davidpdrsn davidpdrsn merged commit fe9c4a0 into main Mar 12, 2023
@davidpdrsn davidpdrsn deleted the david/reduce-state-clones branch March 12, 2023 14:43
davidpdrsn added a commit that referenced this pull request Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants