Atmosphere / wasync

WebSockets with fallback transports client library for Node.js, Android and Java
http://async-io.org
161 stars 47 forks source link

AtmosphereSocket.close does not close underlying AsyncHttpClient #87

Closed jonrimmer closed 10 years ago

jonrimmer commented 10 years ago

Calling close on an Atmosphere socket does not close the underlying instance of AsyncHttpClient. This leaves a worker thread pool and a couple of non-daemon timer threads running, which prevents applications from finishing.

The problem seems to be that AtmosphereSocket sends a close request before it calls its superclass close method. When DefaultSocket.close checks the transportInUse status, it is already CLOSE, so the call to closeRuntime is skipped. This is incorrect - the AsyncHttpClient needs to be closed regardless.

jfarcand commented 10 years ago

@JonRimmer The AsyncHttpClient is shared by default, hence you need to make sure you close it yourself I would think. Try it and let me know. Thanks

jonrimmer commented 10 years ago

It does not appear to be shared. It looks like it's constructed in ClientUtil.create, passed into the socket via its options, stored as a protected socketRuntime field and never published by any public API.

jonrimmer commented 10 years ago

To be clear, the following code...

AtmosphereClient client = ClientFactory.getDefault().newClient(AtmosphereClient.class);

RequestBuilder request = client.newRequestBuilder()
    .method(Request.METHOD.GET)
    .uri("http://localhost/atmosphere")
    .transport(Request.TRANSPORT.WEBSOCKET);

Socket socket = client.create();
socket.open(request.build());
socket.close();

...will internally create a non-shared instance of AsyncHttpClient, when client.create() is called, but will not close it when the socket.close() is called.