Atmosphere / wasync

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

Test case for not closing AHC when using SerializedClient #89

Closed khernyo closed 10 years ago

khernyo commented 10 years ago

The internally created (not shared) AsyncHttpClient is not closed when using SerializedClient. It's probably related to issue #87.

This pull request adds a test case for it which is the same as "ahcCloseTest" except it's using a SerializedClient with DefaultSerializedFireStage.

Let me know if you would rather have an issue opened.

jfarcand commented 10 years ago

That's fine with this issue. I will take a look ASAP. Thanks!

buildhive commented 10 years ago

Atmosphere Framework » wasync #443 UNSTABLE Looks like there's a problem with this pull request (what's this?)

jfarcand commented 10 years ago

Apology for the delay. The test case is invalid as AHC is never set on the client. Just do

        Socket socket = client.create(client.newOptionsBuilder().runtime(ahc).runtimeShared(false).serializedFireStage(new DefaultSerializedFireStage()).build());
khernyo commented 10 years ago

Why would it be invalid?

The test case checks if the AHC created by wasync is closed when the wasync socket is closed. I do not want to create the AHC myself.

Also, if it closes AHC when created with .runtime(ahc).runtimeShared(false), then clearly it should also close AHC without specifying these, because in the latter case there is no way for me to get a hold of AHC to close it myself. No?

khernyo commented 10 years ago

Come on, this issue is valid. Reopen it, at least.

jfarcand commented 10 years ago

@khernyo I don't understand exactly what is the issue? Your test doesn't associate AHC with wAsync, so how can it be closed?

khernyo commented 10 years ago

Oh shit, sorry! You are right, the test case really is invalid. I didn't realize that AHC was not associated with wasync. I don't know how I missed that, because that's my exact problem: when I don't specify a runtime but let wasync create an AHC internally, that AHC will never be closed.

So, I have the following code:

client = ClientFactory.getDefault().newClient(SerializedClient.class);
DefaultSerializedFireStage fireStage = new DefaultSerializedFireStage();
options = client.newOptionsBuilder().serializedFireStage(fireStage).build();
socket = client.create(options);
AtmosphereRequest request = client
               .newRequestBuilder()
               .method(Request.METHOD.GET)
               .enableProtocol(false)
               .uri(url)
               .transport(Request.TRANSPORT.WEBSOCKET)
               .transport(Request.TRANSPORT.LONG_POLLING)
               .build();
socket.open(request);
// Do some communication
socket.close();

After executing socket.close(), I receive the following log message:

[main] WARN org.atmosphere.wasync.impl.DefaultSocket - Cannot close underlying AsyncHttpClient because it is shared. Make sure you close it manually.

I will update the test case shortly.