Fix lifecycle of HttpClient and BlockingHttpClient #11319
Conversation
| @@ -193,6 +193,8 @@ public class ConnectionManager { | |||
| this.clientCustomizer = from.clientCustomizer; | |||
| this.informationalServiceId = from.informationalServiceId; | |||
| this.nettyClientSslBuilder = from.nettyClientSslBuilder; | |||
|
|
|||
| refresh(); | |||
There was a problem hiding this comment.
This is necessary cause this constructor is used for copying ConnectionManager to new instance so we need to bootstrap new instance to setup states. For example AtomicBoolean running is false by default and without refresh() isRunning() will return false. I add this line cause i had failed tests. Hope you will get me, thanks for your review.
| @@ -15,7 +15,7 @@ class ClientHostNameSpec extends Specification { | |||
| e.message.contains('Connect Error: foo_bar') || e.message.contains('Connect Error: No such host is known (foo_bar)') | |||
|
|
|||
| cleanup: | |||
| client.close() | |||
| client.refresh() | |||
There was a problem hiding this comment.
Cause if close this instance other tests will get an already closed client and then throw an exception when trying to use it
There was a problem hiding this comment.
the client in this test is not shared
There was a problem hiding this comment.
Not shared but cached inside factory i guess. When trying to run these tests, I get “closed client” exceptions, when I replace them with refresh, all the tests actually pass. I made quick look up inside the impl of factory client method, and it seems to me that this is caching code, but maybe I'm wrong. I'll check it again and come back with a final answer.
There was a problem hiding this comment.
You're right! these tests failed because of the vpn on my mac. I removed refresh() in this spec.
| @@ -364,7 +364,7 @@ class HttpHeadSpec extends Specification { | |||
| body == "success" | |||
|
|
|||
| cleanup: | |||
| client.close() | |||
| client.refresh() | |||
There was a problem hiding this comment.
Same as previous comment
There was a problem hiding this comment.
this one actually use one shared instance by hashCode
|
@graemerocher @yawkat can you restart build? I think this one failed test is flaky cause it passed on 21 java and on all my local environments |
|
Thanks! |
Fix a bug when a closed client issued an incorrect error as mentioned in #11307
Add special exceptions to simple requests and web sockets
Make tests for it
Add Lifecycle to the BlockingHttpClient interface as well as HttpClient interface
Implement new methods at BlockingHttpClient anonymous class at DefaultHttpClient
Implement new methods at JdkBlockingHttpClient
Fix tests where an undisclosed client was used
JsonStreamSpec: delete "cleanup" with closing clients on each test since the client is shared between all tests and closes automatically at the end(@AutoClosable)
HttpHeadSpec: replace "close" with "refresh" in "cleanup" of one test cause this instance of client is used by other tests
ClientHostNameSpec: replace "close" with "refresh" in "cleanup" of one test cause HttpClient.create() caching clients instances so actually all tests use one instance of client
ByteBufferSpec: make client field @shared cause @Inject fields caching so all test actually use one instance of client