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 shutdown for HTTP/1.1 pipeline #1583

Conversation

dmitrivereshchagin
Copy link
Contributor

Draft fix for #1582.

  • Call cowboy_stream:terminate/3 once for each stream (do not terminate streams from cowboy_http:initiate_closing/2).
  • Prevent next response to be written after "close" option has been sent (check if the last stream ID is reached in cowboy_http:stream_next/1).

@essen
Copy link
Member

essen commented Oct 11, 2022

Isn't a better fix to terminate immediately all streams except the one currently executing (lists:keytake would give us that stream and the remainder to terminate) and then store only that remaining stream in streams in the state.

In any case a test case reproducing the issue should come first. http_SUITE:graceful_shutdown_connection already does some pipelining but the test can likely be enhanced to ensure we get the correct response.

@dmitrivereshchagin
Copy link
Contributor Author

Thanks for your suggestion. Please take a look again.

@dmitrivereshchagin dmitrivereshchagin marked this pull request as ready for review October 11, 2022 23:25
@essen essen added this to the 2.11 milestone Nov 23, 2023
@essen
Copy link
Member

essen commented Dec 5, 2023

Please rebase onto current master so that CI runs properly. I am planning to do a release with this and other things soon.

Sending extra response prevented by terminating all streams except
the one currently executing.
@dmitrivereshchagin
Copy link
Contributor Author

Rebased onto master.

@essen
Copy link
Member

essen commented Dec 18, 2023

This was merged with minor tweaks. Closing, thanks!

@essen essen closed this Dec 18, 2023
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