8000 Fix termination of generators when client connection is closed by dfangl · Pull Request #8582 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Fix termination of generators when client connection is closed #8582

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 27, 2023
Merged

Fix termination of generators when client connection is closed #8582

merged 4 commits into from
Jun 27, 2023

Conversation

dfangl
Copy link
Member
@dfangl dfangl commented Jun 27, 2023 •
edited
Loading

Current Issue

We are currently not closing down our response generators after the client closes the connection. This leads to resource leaks, as the generators keep running in the background, even though no one will ever read the response.

This snipped from hypercorn http_stream.py shows us, that all response send calls will return immediately once the connection is closed.

    async def app_send(self, message: Optional[ASGISendEvent]) -> None:
        if self.closed:
            # Allow app to finish after close
            return

We now need to also stop the generators, once we detect the client connection is closed.

Changes

  • Access internals of send call to determine if the connection to the client is closed already and raise a BrokenPipeError in that case.
  • If any ConnectionError is raised during sending data from the iterator, we stop reading from it and close the (async) generator
  • Add test for this behavior in the asgi bridge, to avoid future regression
  • We do not read all responses in our request logging, to enable chunked responses for requests not parsed by the serializer

@dfangl dfangl requested a review from thrau as a code owner June 27, 2023 18:24
@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jun 27, 2023
@coveralls
Copy link

Coverage Status

coverage: 82.719% (+0.06%) from 82.664% when pulling 0e218e0 on fix-generator-termination into 59cab78 on master.

thrau
thrau approved these changes Jun 27, 2023
8000
View reviewed changes
Copy link
Member
@thrau thrau left a comment

Choose a reason for hiding this comment

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

great find! this was a fun session for me also 😁

@thrau thrau merged commit 0088cd7 into master Jun 27, 2023
@thrau thrau deleted the fix-generator-termination branch June 27, 2023 19:39
This was referenced Jun 28, 2023
Closed
Closed
@alexrashed alexrashed mentioned this pull request Jul 5, 2023
Closed
2 tasks
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
@alexrashed alexrashed mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thrau thrau thrau approved these changes

Assignees
No one assigned
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@dfangl @coveralls @thrau
0