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

Remove duplication in serving with and without graceful shutdown #2803

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

jplatte
Copy link
Member

@jplatte jplatte commented Jun 22, 2024

Simpler implementation of the same idea from #2478, against latest main.

@jplatte jplatte force-pushed the jplatte/reduce-serve-duplication branch from 7047547 to 04b729e Compare July 18, 2024 09:32
Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

This is great.

I just wonder if this couldn't go one step further and merge the Serve and WithGracefulShutdown into a single generic type. As it stands now, Serve<M, S> is just WithGracefulShutdown<M, S, Pending>` so why not embrace it?

The only difference I know of is that with the current separation users cannot call with_graceful_shutdown multiple times but I don't think that would be a problem.

@jplatte
Copy link
Member Author

jplatte commented Jul 19, 2024

Sure, could do that. But it feels like leaking implementation details into the public API, and I don't see the little bit of extra code as much of a problem.

@jplatte jplatte force-pushed the jplatte/reduce-serve-duplication branch from f9308cf to 3734152 Compare September 26, 2024 13:41
@jplatte
Copy link
Member Author

jplatte commented Sep 26, 2024

I'm so confused about that error. Need to look into it, it's definitely no flake.

@mladedav
Copy link
Collaborator

I've taken a look. It's because the graceful shutdown serve code has additional tracing with trace!("connection {remote_addr} accepted"); and another one for closing the connection, but this test tries to parse each tracing line as a json with excpected fields.

So the easiest solutions are to pass the filter in tracing_helpers.rs as a function argument and pass axum::rejection=trace instead of the current axum=trace filter.

Right now the helper is used from just this one test so it could also just be changed in place, but passing the filter in is a bit cleaner.

The previous code would have not worked with the inner future migrating
to a different OS thread within a work-stealing async executor.
@jplatte
Copy link
Member Author

jplatte commented Sep 27, 2024

Thanks for investigating!
I've pushed a potentially overengineered fix, let me know what you think 😄

Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

Chaining the with_filter call certainly looks better on the callsite than just passing it in, so this is good with me. Even if a custom future impl maybe is a bit complex solution for a test helper.

pub(crate) struct CaptureTracing<T, F> {
f: F,
filter: Option<Targets>,
_phantom: PhantomData<fn() -> T>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, why did you do this? I assume that since it's DeserializeOwned, the T will not be a reference so I don't see the point of going out of your way to make it contravariant. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

fn(T) would be contravariant, fn() -> T is covariant. It's just good form for generic "output" parameters, doesn't actually matter here. See https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns for details.

@jplatte jplatte merged commit 5d8541d into main Sep 27, 2024
16 of 18 checks passed
@jplatte jplatte deleted the jplatte/reduce-serve-duplication branch September 27, 2024 21:16
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.

2 participants