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

Ensure response filters are not left waiting for buffered body #1353

Merged
merged 2 commits into from Oct 21, 2022

Conversation

artgon
Copy link
Contributor

@artgon artgon commented Oct 20, 2022

In case of client cancellations or connectivity errors, we may have a response filter chain hanging, waiting for a complete buffered body. The goal for this PR is to close out that filter chain and clean up any body chunks being processed.

@@ -121,15 +122,18 @@ private void sendResponse(final StatusCategory statusCategory, final int status,
headers.add("Content-Length", "0");
zuulResponse.finishBufferedBodyIfIncomplete();
responseFilterChain.filter(zuulResponse);
fireEndpointFinish(true);
fireEndpointFinish(true, ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

One slight nuance here with the change to fireEndpointFinish, since we close out the body on L123 (zuulResponse.finishBufferedBodyIfIncomplete()) and fire into the response filter chain, I don't think the new finishResponseFilters function will have an issue as no filter should be waiting for a body - so will just be a no-op? Just double checking 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but in the case of that static response, it won't pop into the if block that checks if the filter chain is waiting for a buffered body.

@artgon artgon merged commit 9e7c086 into master Oct 21, 2022
@artgon artgon deleted the finish-outfilters branch October 21, 2022 16:06
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.

None yet

4 participants