Interest in support for .send()-ing a stream and receiving a message as a stream?

[loganfsmyth]

• I've searched for any related issues and avoided creating a duplicate
  issue.

Description

For receiving larger payloads in messages, it's not ideal that you end up having to worry about maxPayload and having buffer it all in memory before emitting the "message" event, and for sending larger payloads, it is a bit of a pain to manually chunk things up and to manually coordinate the 'fin' bit. Using streams also means that we get backpressure support which would be wonderful.

I was thinking of playing around with implementing support for .send() to accept a Readable stream, and for message events to emit a Readable stream, if a flag were set. binaryType could potentially be reused, but I could also see it being nice to stream text messages, so maybe a separate textType: "stream" would be desirable.

Thoughts? Has this been discussed anywhere in the past? I looked but everything stream related seems to be about WebSocketStream.

[lpinca]

I prefer to keep websocket.send() simple and compatible with the browser interface. Users interested in sending a "stream" can consume the Readable stream in the application code and send each chunk with websocket.send(). I think this is easier and gives developers more freedom.

websocket.send() was accepting a Readable stream but support for it has been removed in #875 because the implementation was buggy and did not support backpressure.

I also think it does not make sense to have listeners of the 'message' event accept a Readable stream argument. The WebSocket protocol is message based and each message is binary or textual per spec. Each 'message' event has to correspond to a message sent by the other peer.

[loganfsmyth]

You're right about send being able to avoid it by manually sending packets, but it does seem a lot nicer, especially as a nice pairing if the message event handled the same.

I also think it does not make sense to have listeners of the 'message' event accept a Readable stream argument. The WebSocket protocol is message based and each message is binary or textual per spec. Each 'message' event has to correspond to a message sent by the other peer.

The websocket protocol is indeed message-based, but those messages can have any size, so maxPayload ends up enforcing a limit that is not at all required by the spec. I don't think I follow your logic for message-based correlating with the messages being small.

In my context, I'm sending files over a websocket as one message (potentially many frames of course). With send, I can manually split it into chunks, but for receiving the message, ws offers no way to take that data and write it to a file as it arrives, so Node ends up having to hold the whole file in RAM beforehand, which is very much not ideal.

[lpinca]

You can set maxPayload to 0 and have unlimited size messages.

With send, I can manually split it into chunks, but for receiving the message, ws offers no way to take that data and write it to a file as it arrives, so Node ends up having to hold the whole file in RAM beforehand, which is very much not ideal.

This is true if you use fragmented messages. The 'message' event is not emitted until the whole message is received and this is by design. I don't want to deviate from the browser interface with a 'fragment' event or something like that.
Instead of using fragmented messages you could use a message for each chunk.

The first message could be an info message with the file name and size in bytes. Subsequent messages could be the file contents, one for each chunk read from the Readable file stream. When the number of bytes received via 'message' events corresponds to the original file size, you know that the whole file has been received. You can do this with the streaming interface to have easy backpressure support. There is minimal overhead (only the initial info message) and no buffering on the receiving end.

[smhk]

@loganfsmyth , that is an interesting idea, but, since you are asknig for opinions:

• Unfortunately we would be strongly opposed to this.

the reason is easy to explain. There is only ONE thing we want from the amazing /ws. RELIABILITY!

I can easily write a few lines of code to deal with issues such as long messages, etc. All I want from the amazing /ws is continued total reliability!

I hope that makes sense. Just one opinion!

[loganfsmyth]

You can set maxPayload to 0 and have unlimited size messages.

My biggest goal is to not have to hold the whole message in memory, so while this will work, it won't achieve that goal if many clients were to send large files at the same time, it could overwhelm the box quickly if we did this.

The first message could be an info message with the file name and size in bytes. Subsequent messages could be the file contents, one for each chunk read from the Readable file stream. When the number of bytes received via 'message' events corresponds to the original file size, you know that the whole file has been received. You can do this with the streaming interface to have easy backpressure support. There is minimal overhead (only the initial info message) and no buffering on the receiving end.

You're right that it's an option, but I'd rather not be pushed to define my messaging format based on the limitations of ws so that's what I wanted to avoid. I'd been planning to send one JSON message with the file metadata followed by a binary message immediately after it with the file content, sending the file as many fragments as they are read from disk. Sending multiple messages negates the purpose of message fragments as far as I can tell, and it complicates parsing the messages as they come in because there's no longer a simple pair of messages, and instead a stream of messages that would need to define their own framing protocol on top of Websocket's existing framing.

the reason is easy to explain. There is only ONE thing we want from the amazing /ws. RELIABILITY!

I'm not sure what definition of reliable you have in mind, but I'd consider the requirement that a message be fully aggregated in memory to be detrimental to reliability because you more easily run the risk of overloading the RAM of the machine. Node itself is heavily dependent on streams as a concept, so I'm just generally surprised that they aren't supported as a first-class type in WS.

[lpinca]

I'd rather not be pushed to define my messaging format based on the limitations of ws

I'd rather say that it's a limitation of the WebSocket API as defined by the HTML spec not ws. We follow that. A message is made of all fragments not a single one. Emitting a 'message' event with a stream argument on the first fragment received does not make much sense in my opinion because it's a big breaking change and a spec deviation. I'd rather add a new 'fragment' event but this will not solve the fragmented messages buffering issue, not without changing the behavior of the 'message' event.

I'd been planning to send one JSON message with the file metadata followed by a binary message immediately after it with the file content, sending the file as many fragments as they are read from disk. Sending multiple messages negates the purpose of message fragments as far as I can tell, and it complicates parsing the messages as they come in because there's no longer a simple pair of messages, and instead a stream of messages that would need to define their own framing protocol on top of Websocket's existing framing.

It is not that different. Compare your ideal solution which sends many fragments to something like this that sends many messages.

// Writing end.

const info = JSON.stringify({ filename: 'big.iso', size: 10737418240 });
const websocketStream = WebSocket.createWebSocketStream(ws);

ws.send(info, function (err) {
  if (err) {
    handleError(err);
    return;
  }

  const fileStream = fs.createReadStream('big.iso');

  fileStream.pipe(websocketStream, { end: false });
});

// Reading end.

let filename;
let size;
let bytesRead = 0;
let fileStream;

const websocketStream = WebSocket.createWebSocketStream(ws);

websocketStream.on('data', function (chunk) {
  if (typeof chunk === 'string') {
    ({ filename, size } = JSON.parse(chunk));
    fileStream = fs.createWriteStream(filename);
  } else {
    bytesRead += chunk.length;

    if (!fileStream.write(chunk)) {
      websocketStream.pause();
      fileStream.on('drain', function () {
        websocketStream.resume();
      });
    }

    if (bytesRead === size) {
      fileStream.end();
      bytesRead = 0;
    }
  }
});

[loganfsmyth]

I'd rather say that it's a limitation of the WebSocket API as defined by the HTML spec not ws. We follow that.

How do you define when you do and don't follow the spec? The spec doesn't return a boolean from send but ws does. The spec doesn't have .terminate(), or .ping() or .ping(). There's plenty of stuff supported in ws that is specifically meant to support Node and server usecases that are not defined in the HTML spec, because they are necessary for implementing an effective WebSocket server.

A message is made of all fragments not a single one.

I'd say that a message is a sequence of fragments, but how that sequence is exposed is entirely up to the library. The current behavior absolutely makes sense as the default, but that doesn't mean it needs to be limited to that. All of the current behaviors are trivially implementable on top of a stream of fragments, but not the other way around.

Emitting a 'message' event with a stream argument on the first fragment received does not make much sense in my opinion because it's a big breaking change and a spec deviation. I'd rather add a new 'fragment' event but this will not solve the fragmented messages buffering issue, not without changing the behavior of the 'message' event.

I'm not trying to say this would be the default or a breaking change, I'm proposing it as an opt-in feature, since I do agree that for many usecases, the streaming aspect of things is not important. I don't personally see much benefit to a fragment event when streams already provide an interface for a sequence of data in a well-defined way for Node.

It is not that different. Compare your ideal solution which sends many fragments to something like this that sends many messages.

It is primarily the bytesRead part that I disagree about. For a generic stream of data, you may not know the length at the time of sending the data. Maybe it's not a file, maybe it's being dynamically generated, or proxied through from some other TCP socket. Given that, the only approach is to the build your own framing protocol within the binary message, which then ends up duplicating essentially exactly the same fin logic that Websockets fragments already implement.

[lpinca]

How do you define when you do and don't follow the spec?

browser clients return the full message when a fragmented message is received.

The spec doesn't return a boolean from send but ws does. The spec doesn't have .terminate(), or .ping() or .ping()

No, ws does not return a boolean from websocket.send() and yes there are some deviations from the spec because there is no other way to do it. There is no way to send fragmented messages from a browser or ping / pong frames.

I'm not trying to say this would be the default or a breaking change, I'm proposing it as an opt-in feature, since I do agree that for many usecases, the streaming aspect of things is not important. I don't personally see much benefit to a fragment event when streams already provide an interface for a sequence of data in a well-defined way for Node.

Returning a Readable stream instead of a full message adds a lot of complexity for a use case that can easily be handled via normal messages and the streaming interface instead of a fragmented message:

1. How do you handle backpressure if the Readable stream is paused or piped to a slower stream?
2. What to do if the stream is destroyed?
3. How do you check that the message is correctly UTF-8 encoded? Currently that is done when the full message is received. There is no streaming UTF-8 validation. Returning a stream only for binary messages would be inconsistent.

[loganfsmyth]

browser clients return the full message when a fragmented message is received.

Because the spec was written before streams in the browser were an option. I think in the context of streams being common in Node, it's an understandable request.

No, ws does not return a boolean from websocket.send() and yes there are some deviations from the spec because there is no other way to do it. There is no way to send fragmented messages from a browser or ping / pong frames.

Ah my mistake, I misremembered about .send, I guess what I was thinking of is the fact that it accepts a callback, which the browser API does not. The core part being that .send handles backpressure in a way that is totally different from the browser API.

My point is, clearly you've expanded the API to include things that are non-standard, so it doesn't seem like supporting streams is necessarily beyond scope here.

Returning a Readable stream instead of a full message adds a lot of complexity for a use case that can instead easily be handled via normal messages and the streaming interface instead of a fragmented message:

Can you elaborate on the complexity? It is managing the async state transitions beyond the existing cases that is the concern?

1. How do you handle backpressure if the Readable stream is paused or piped to a slower stream?

I'd expect that Receiver would delay calling the callback passed to _write until the data is pushed into the readable stream, and if push returns false, the callback would not be called until _read is triggered on the readable stream.

2. What to do if the stream is destroyed?

I'd probably say discard further fragments that arrive in that message.

3. How do check that the message if correctly UTF-8 encoded? Currently that is done when the full message is received. There is no streaming UTF-8 validation. Returning a stream only for binary messages would be inconsistent.

I haven't looked into that, but it seems like ws would have to validate the fragments as they arrive, carrying over any incomplete utf8 sequences to prepend to the next fragment, like Node's StringDecoder does. If the message is incorrect, the stream can error and the connection would close, similar to now. I guess it's a bit unclear whether the error would be emitted on both the socket and the readable stream though.

[lpinca]

Can you elaborate on the complexity? It is managing the async state transitions beyond the existing cases that is the concern?

Yes, Node.js streams are complex and have a lot of edge cases. This feature also affects the current streaming interface as the wrapping Duxplex stream should handle cases where message is a Readable stream.

I'd probably say discard further fragments that arrive in that message.

There are some edge cases that must be handled correctly to avoid dead locks and to ensure that the callback of receiver._write() is always called. For example we would also need a listener for the 'close' event on the Readable object as _read() is not called if the stream is destroyed.

The Readable stream must be destroyed if an error occurs or if websocket.terminate() is called while the Readable stream is being consumed.

Probably a lot of other things I can't think of at the moment.

I haven't looked into that, but it seems like ws would have to validate the fragments as they arrive, carrying over any incomplete utf8 sequences to prepend to the next fragment, like Node's StringDecoder does.

Yes but there is no such validator at the moment, it must be written.

Overall it's a lot of work and complexity that is not needed in my opinion.

[loganfsmyth]

Fair enough. Sounds like this is a no-go. I appreciate you taking the time to talk to me about it.

[lpinca]

Thank you for the constructive discussion.