search

codeminders / socket.io-server-java

Java Backend for Socket.IO (http://socket.io/)
36 stars 18 forks source link

Optional websocket transport synchronization #14

Closed nivertius closed 6 years ago

nivertius commented 6 years ago

Depending on this parameter websocket transport "send" methods are either wrapped synchronized block or not. Change was made due to invalid socket state exception (FULL_TEXT_WRITING) on simultaneously emitted messages via same websocket instance.

alexsaveliev commented 6 years ago

Hi @nivertius,

Thanks for the patch! I think, however, that the decision if send should be synchronized or not should not depend on configuration value. I'd rather process the following way: Extend WebsocketIOServlet - to return SynchronizedWebsocketTransportProvider. SynchronizedWebsocketTransportProvider - returns SynchronizedWebsocketTransport. SynchronizedWebsocketTransport - returns SynchronizedWebsocketTransportConnection SynchronizedWebsocketTransportConnection - wrap there calls of super.sendXXX into synchronized block WDYT?

nivertius commented 6 years ago

Hello @alexsaveliev,

Thanks for quick response.

Actually, we tried that before forking. :smiley:

WebsocketTransportConnection is a final class, and for a reason: it has ServerEndpoint annotation, and is actually created by application container. We've tried overriding configuration, but that didn't help either.

This was, IOHO, the best way to solve this, but we are open for suggestions.

alexsaveliev commented 6 years ago

Let's make it synchronized then?

nivertius commented 6 years ago

Synchronize methods sendXXX in WebsocketTransportConnection without possibility of configuration? This might not be exactly the solution everybody wants, because if they have some kind of external synchronization, or thier buisness logic assures that multiple messages won't be sent at once, this synchronization is redundant or unnecessary.

alexsaveliev commented 6 years ago

How about cfdc1c157ea2340d87d1e25a6cf6d96d20241b5f? If you are fine with it, I'll merge my changes to master and then we'll bump artifact

nivertius commented 6 years ago

This solves our problem, but I'm still in favor of configuration in servlet parameters instead of static field. I'll rebase @tomasztomasztomasz commit with your changes in a minute.

nivertius commented 6 years ago

After second thought, your solution is fine, when you are doing jar scanning for servlets.

We would be grateful, if you'd merge cfdc1c1 and release new version to central.

alexsaveliev commented 6 years ago

1.0.6 should be in maven central now, please test