Skip to content

Fix OkHTTP client transport leak - #11054

Merged
ejona86 merged 1 commit into
grpc:masterfrom
hypnoce:okhttp_insuestate_leak
Mar 29, 2024
Merged

Fix OkHTTP client transport leak#11054
ejona86 merged 1 commit into
grpc:masterfrom
hypnoce:okhttp_insuestate_leak

Conversation

@hypnoce

@hypnoce hypnoce commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

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

@ejona86 ejona86 left a comment

Copy link
Copy Markdown
Member

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?

Comment thread okhttp/src/main/java/io/grpc/okhttp/OkHttpClientTransport.java Outdated
Comment thread okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java Outdated
@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

hypnoce commented Mar 29, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
+1 more reviewer
@temawi temawi temawi approved these changes
Reviewers whose approvals may not affect merge requirements

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