-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix httpclient5.x connection leak #3727
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: jiangyuan <[email protected]>
b6e8de9
to
ff5edf2
Compare
So this change breaks the build in the retry tests.
|
Hmm, locally they pass. Rerunning build. |
Any idea how to test this? The test in the reproducer in #3696 doesn't pass with this fix. |
5428e24
to
eb28861
Compare
Signed-off-by: joecqupt <[email protected]> Signed-off-by: jiangyuan <[email protected]>
eb28861
to
6ce0e1d
Compare
f0dfe66
to
86e28b0
Compare
Signed-off-by: jiangyuan <[email protected]> test ci build
86e28b0
to
aeffc5e
Compare
the reproducer code with some error config . the route uri not match mock server . |
@spencergibb @ryanjbaxter PTAL |
Could we use the reproducer as a basis for a test? |
Signed-off-by: jiangyuan <[email protected]>
Signed-off-by: jiangyuan <[email protected]>
i have use the reproducer code as a basis add a unit test . Let me explain the code which i modified: first i add
but this changed the underlying implementation of so i add config to
After modifying the config , I modified two associated unit tests: Finally i add a unit test to test httpcommonts as the underlying implementation of @ryanjbaxter @spencergibb PTAL |
Can you elaborate on why you needed to add |
because the reproducer code use httpclient5 as the underlying implementation of ProxyExchange issue : #3696 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JoeCqupt unfortunately, adding the Apache httpclient 5 dependency will change the underlying http client for all the tests. You would need to add a new integration test module to not pollute the classpath here https://github.com/spring-cloud/spring-cloud-gateway/tree/main/spring-cloud-gateway-integration-tests
OK i will add a new integration test |
#3696