zwopple / PocketSocket

Objective-C websocket library for building things that work in realtime on iOS and OS X.
Other
414 stars 129 forks source link

Queue messages sent before websocket was open. #48

Closed tcard closed 6 years ago

tcard commented 8 years ago

Nothing stopped you from sending a message before the handshake is complete and the websocket is ready. Before this change, doing so could cause a crash if you were unlucky.

I have a codebase that relies on code like this to work:

PSWebSocket *socket = [PSWebSocket clientSocketWithRequest:request];
[socket open];
[socket send:@"test];

(I was using SwiftWebSocket before.)

I added a queue of messages sent before the handshake is complete, which makes that safe.

robertjpayne commented 8 years ago

While I understand the convenience here I'm against adding this as a supported API and would rather applications implement queueing logic suites there needs for a few reasons:

  1. This particular implementation will have race conditions in it. The way the messages are enqueued and the way the delegate is notified of a connection opening it wont be long until some weird undefined behaviour crops up for someones specific application because the initial messages were sent interleaved with additional messages.
  2. It obscures the state of the websocket, webSocketDidOpen really means, it just finished it's handshake and nothing else has taken place yet.
  3. If a websocket fails to connect, and you retry and it re-connects you should probably repurpose already created message packets rather than re-creating and enqueuing them. Application logic is the best place to create that queue because PSWebSocket's are one time use objects.

Open to rebuttals on these three points but I'm trying to balance clarity of what the socket does vs developer convenience that is easily implemented at the application level.

robertjpayne commented 8 years ago

To be fair, right now it's probably a bit obscure, you can call send but depending on your setup it could cause havoc since the behaviour is undefined. As a result once this discussion is finished I'll either implement the queueing logic, or fire an exception if you send messages too soon.

tcard commented 8 years ago

I see your points, and throwing an exception on premature sends is also a good option. It certainly would make a simpler API, at the expense of a bit more of boilerplate on the user's end. So, up to you.

robertjpayne commented 8 years ago

@tcard yea I do get it's a bit of boilerplate for some users, it really comes down to exactly the needs of the application and avoids us trying to match all the queueing needs. I'll add the exception in though so it's clear that you must provide your own queueing logic.