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

Java client does not terminate gracefully anymore #409

Open
sbordet opened this Issue Feb 19, 2013 · 3 comments

Comments

Projects
None yet
1 participant
@sbordet
Member

sbordet commented Feb 19, 2013

In previous versions of CometD and Jetty websocket, the default thread pools of the long polling and websocket transports were started as daemon threads. Not anymore. Now my java client cannot terminate gracefully anymore.
Ok, the LongPollingTransport can be reconfigured to use a daemon thread pool by passing a custom HttpClient with custom executor/scheduler instances. The websocket client (9.0.0.RC0), however, instantiates a ScheduledExecutorScheduler without daemon threads internally. There is no way to reconfigure it.

I think, by default the thread pools should still be created as daemon threads. If there are good reasons not to do so, I'd propose one of the following options:
a) introduce a simple boolean option to the transports, whether to use daemon thread pools or not
b) at least introduce a setter on WebSocketClient (jetty) that allows setting the scheduler (not present in websocket-client-9.0.0.RC0) - I think this has to be done anyway
c) include kind of a shutdown() method in BayeuxClient that iterates over all transports and calls a new shutdown() method on each transport. The corresponding implementation calls stop() on the underlying client (HttpClient, WebSocketClient). That seems also to terminate the underlying clients including non-daemon thread pools.

I had posted the issue on the mailing list: https://groups.google.com/d/msg/cometd-users/7agxLpVthuE/4AJlhFfyZAgJ

@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Feb 19, 2013

Member

Originally reported by fdummert on 2013-02-19T03:54:15Z

Member

sbordet commented Feb 19, 2013

Originally reported by fdummert on 2013-02-19T03:54:15Z

@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Feb 19, 2013

Member

sbordet on 2013-02-19T06:50:11Z:
Florian,

I agree with you that default pools needs to be daemon since you can't configure them explicitly.

However, it is unclear to me what exact version of CometD you're using.
The Jetty 9 WebSocket client only works with CometD 3, so is this bug affecting CometD 3 or, as you reported, 2.5.1 ?

Thanks !

Member

sbordet commented Feb 19, 2013

sbordet on 2013-02-19T06:50:11Z:
Florian,

I agree with you that default pools needs to be daemon since you can't configure them explicitly.

However, it is unclear to me what exact version of CometD you're using.
The Jetty 9 WebSocket client only works with CometD 3, so is this bug affecting CometD 3 or, as you reported, 2.5.1 ?

Thanks !

@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Feb 19, 2013

Member

fdummert on 2013-02-19T07:26:04Z:
Sorry for the confusion. I actually used CometD 2.5.1, patched for jetty 9.0.0.RC0 (my fault - when I searched for changes in github, I totally missed some major source code changes since 2.5.1). So this issue should have been targeted to CometD trunk (2.6? 3.0?).
Now I've seen your answer on the post mentioned above. There you are explaining that a client needs to call stop() explicitly on the transports. Agreed.

I still think, that WebSocketClient should get a setScheduler.

And what about c), this could still be a handy utility method?

Member

sbordet commented Feb 19, 2013

fdummert on 2013-02-19T07:26:04Z:
Sorry for the confusion. I actually used CometD 2.5.1, patched for jetty 9.0.0.RC0 (my fault - when I searched for changes in github, I totally missed some major source code changes since 2.5.1). So this issue should have been targeted to CometD trunk (2.6? 3.0?).
Now I've seen your answer on the post mentioned above. There you are explaining that a client needs to call stop() explicitly on the transports. Agreed.

I still think, that WebSocketClient should get a setScheduler.

And what about c), this could still be a handy utility method?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment