search

status-im / nim-websock

Websocket for Nim
82 stars 15 forks source link

properly support concurrent message sending #126

Closed etan-status closed 2 years ago

etan-status commented 2 years ago

nim-websock suffered from a number of issues that are being addressed:

  1. Long messages > frameSize (default 1 MB) were split into fragments of frameSize each. However, when a concurrent message is sent, it may be interleaved among the fragments of an already-sending message. This is only allowed for control packets without a mux extension.

  2. When the WebSocket session is closed, a msg may have been partially received. This partial frame was reported as a full message, without indication that the receiving was canceled. This behaviour is fixed by raising a WSClosedError instead of reporting the partial msg.

  3. When an individual send operation was canceled, it would actually stop sending the remainder of a potentially partially sent messages. This would corrupt the stream for concurrent and followup operations. Cancellation is now inhibited for the message currently sending. It is still possible to cancel messages that are not yet scheduled.

  4. Messages could get reordered when using asynchronous encoders. This is addressed by delaying followup messages until the current message is fully encoded and transmitted (except for control packets).

etan-status commented 2 years ago

Also related to nimbus-eth2 usage: https://github.com/status-im/nim-json-rpc/pull/146

etan-status commented 2 years ago

I did prolonged manual testing on nimbus-eth2 as part of https://github.com/status-im/nimbus-eth2/pull/4061 and nim-websock was very reliable.