Skip to content

Fix OkHTTP client transport leak #11054

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
ejona86 merged 1 commit into from
Mar 29, 2024
Merged

Fix OkHTTP client transport leak #11054

ejona86 merged 1 commit into from
Mar 29, 2024

Conversation

@hypnoce
Copy link
Contributor

@hypnoce hypnoce commented Mar 28, 2024

Fixes #11053: make sure to remove finished stream in okhttp client transport even if a pending stream was started.

@hypnoce hypnoce force-pushed the okhttp_insuestate_leak branch 2 times, most recently from 8c1b9e0 to 41f2557 Compare March 28, 2024 10:42
ejona86
ejona86 reviewed Mar 28, 2024
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Wow, this looks like a pretty old bug. Seems it would mostly break idleTimeout() and only if MAX_CONCURRENT_STREAMS was reached. The memory leak could become noticeable if connections lived long enough and the connection remained limited by MAX_CONCURRENT_STREAMS.

What broke for you to notice this?

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 28, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 28, 2024
@hypnoce
Copy link
Contributor Author

hypnoce commented Mar 29, 2024 •
edited
Loading

Wow, this looks like a pretty old bug. Seems it would mostly break idleTimeout() and only if MAX_CONCURRENT_STREAMS was reached. The memory leak could become noticeable if connections lived long enough and the connection remained limited by MAX_CONCURRENT_STREAMS.

What broke for you to notice this?

We have a grpc server with MAX_CONCURRENT_STREAM set to 100 and a single client with 512 threads doing parallel requests to the server using the same okhttp channel. And the client was going out of memory.

@hypnoce
Fixes grpc#11053: make sure to remove finished stream in okhttp clien…
19f970f 
…t transport even if a pending stream was started.
@hypnoce hypnoce force-pushed the okhttp_insuestate_leak branch from 41f2557 to 19f970f Compare March 29, 2024 07:58
@ejona86 ejona86 requested a review from temawi March 29, 2024 15:22
@ejona86 ejona86 added kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run TODO:backport PR needs to be backported. Removed after backport complete labels Mar 29, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 29, 2024
@ejona86 ejona86 merged commit d21fe32 into grpc:master Mar 29, 2024
This was referenced Mar 29, 2024
Merged
Merged
@YifeiZhuang YifeiZhuang removed the TODO:backport PR needs to be backported. Removed after backport complete label Apr 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ejona86 ejona86 ejona86 approved these changes

@temawi temawi temawi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OkHttp stream leak when hitting MAX_CONCURRENT_STREAMS

5 participants

@hypnoce @ejona86 @temawi @YifeiZhuang @grpc-kokoro