-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
OkHttp responses not closed properly #4666
Comments
:( it seems we might have introduced a regression when refactoring our OkHttp HttpClient implementation. We'll need to release a patch with a fix. |
Continues discussion in #4660 (reply in thread) I'm not sure that the handler causing the leakage is the one for the
It does seem that reading the I'm trying to see where else we can be leaking the connection by not closing the response. |
There are only 2 calls now. Either one using asyncbody, or a websocket. For there to be an issue with websockets, sendClose would not have to be called - as far as I'm aware non of that logic changed. The change wrt asyncbody is that previously the sendAsync results were directly materialized by okhttp, now they are not.
Isn't the reporting based upon the stack that was captured for a connection that was leaked?
I've confirmed the fix works - that is you can easily reproduce this scenario doing repeated reads. At least with an integration test / real server (if you run against the mock server http1 will be used, I haven't confirmed if it has the same problem). What I see is that when Http2Codec.read is called under exhausted, endOfInput is only called if an IOException is thrown from the FramingSource. If it's just -1 being returned, then endOfInput is not called - so the auto close is not triggered. Some of the http1 logic may be affected as well because I see the same read method in Http1Codec$AbstractSource.read. |
The reporting just says that a connection was leaking, not which one (at least this is what I get from reading the code in
I haven't checked with HTTP2, but I've validated that the connection is actually deallocated when read to exhaustion with the mock server (HTTP1). I'll check with HTTP2 and see if I can provide a more realistic test to verify the fix. |
So it's confirmed, the issue is only present when using the HTTP 2 protocol. The PR (#4665) includes a test that verifies this against a fine-tuned OkHTTP Mock Server. |
### What changes were proposed in this pull request? This PR aims to upgrade K8s client to 6.3.1. I tested manually that the regression is fixed. - https://github.com/fabric8io/kubernetes-client/releases/tag/v6.3.1 - https://github.com/fabric8io/kubernetes-client/releases/tag/v6.3.0 ### Why are the changes needed? SPARK-41521 tried to use 6.3.0, but it was reverted due to fabric8io/kubernetes-client#4666. v6.3.1 fixed it. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the Cis. Closes #39094 from dongjoon-hyun/SPARK-41552. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
Describe the bug
With 6.3.0 release, it seems that some OkHttp client responses are not closed properly and every 5 minutes, we get bunch of warnings about them in our logs in the Strimzi project For example:
This was also discussed here
Fabric8 Kubernetes Client version
6.3.0
Steps to reproduce
Just using the Fabric8 Kubernetes client. If needed, I can try to create some reproducer code snippets. But it seems to come from all kind of places. The stacktraces are in the description.
Expected behavior
No warnings about unclosed messages will appear in the logs.
Runtime
Kubernetes (vanilla)
Kubernetes API Server version
1.25.3@latest
Environment
Linux
Fabric8 Kubernetes Client Logs
See the description. I can provide more logs if needed.
Additional context
n/a
The text was updated successfully, but these errors were encountered: