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

fix(http1): http1 server graceful shutdown fix #3233

Closed
wants to merge 2 commits into from

Conversation

rob2244
Copy link
Contributor

@rob2244 rob2244 commented May 23, 2023

fix issue in the graceful shutdown logic
which causes the connection future to hang
when graceful shutdown is called prior to any
requests being made. This fix checks to see
if the connection is still in its initial state
when disable_keep_alive is called, and starts the
shutdown process if it is.

This addresses issue #2730

fix issue in the graceful shutdown logic
which causes the connection future to hang
when graceful shutdown is called prior to any
requests being  made. This fix checks to see
if the connection is still in its initial state
when disable_keep_alive is called, and starts the
shutdown process if it is.

This addresses issue hyperium#2730
@rob2244
Copy link
Contributor Author

rob2244 commented May 23, 2023

I'm not sure if this is the best solution or not, it feels like it's catering a little too much to this one edge case if you know what I mean. But it does pass all the tests. Let me know if there's a better way to do this.

@rob2244
Copy link
Contributor Author

rob2244 commented May 23, 2023

Actually, it seems like the tests failed on mac-OS. I think this solution may introduce a race condition.

@seanmonstar seanmonstar requested a review from sfackler May 23, 2023 22:20
@seanmonstar seanmonstar linked an issue Jul 6, 2023 that may be closed by this pull request
@seanmonstar
Copy link
Member

Ok, after taking a look: the problem is that the connection is closing on all platforms, but macOS is triggering an error trying to write the second half of the request.

@sfackler in your mind, what behavior did you expect? If the request has already started, as in we've received some of the bytes, but not the full headers, should that be allowed to continue? Or should that be closed, and we only allow requests to continue if they are streaming their body?

I think the goal was to prevent pre-emptive idle connections from keeping the server alive, not to interrupt new connections that had their headers split between packets, right?

@seanmonstar
Copy link
Member

Fixed this up in #3261.

@seanmonstar seanmonstar closed this Jul 6, 2023
@sfackler
Copy link
Contributor

sfackler commented Jul 6, 2023

Yeah, I would expect that the idle timer would start counting as soon as the connection is created, and stop as soon as the first byte of the headers are received. The header timeout should handle the second case I think.

@rob2244 rob2244 deleted the graceful-shutdown-fix branch July 15, 2023 16:53
@rob2244 rob2244 restored the graceful-shutdown-fix branch July 15, 2023 16:53
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.

Connection::graceful_shutdown always waits for the first request.
3 participants