Make #flush in WebSocket#stream a no-op to not send wrongly frames

[spuun]

Fixes #16532

[ysbaddaden]

This is changing the behavior of HTTP::WebSocket#stream that explicitly documents that StreamIO#flush will send any data in the buffer immediately.

If I'm reading correctly, #write can return with pending data in @buffer that we might want to flush manually for some reason.

Now that we have a proper #send_data method, maybe #flush could write pending data from @buffer when called? Of course it shall do nothing when empty and certainly not set the FIN flag on the frame.

Note: #write could then call #flush internally when the buffer is full.

[ysbaddaden]

Unless this is still exposing the same issue: we can receive "DAT" then "A" (flush) then an empty frame with the FIN flag?

[spuun]

Unless this is still exposing the same issue: we can receive "DAT" then "A" (flush) then an empty frame with the FIN flag?

Flush will send FIN, then the next flush will send an empty FIN. However, the empty FIN will have opcode set to CONTINUATION which is why my browser disconnected the socket (so i never saw the empty message in the browser).

From the docs

The flush method sends all the data in io and resets it. The remaining data in it is sent as a message when the block is finished executing

IMO this is wrong. It will send what's in the buffer and "end" the first message (FIN). But then it doesn't really "reset" the stream, because the opcode will still be CONTINUATION which is wrong when the next message is being started.

[straight-shoota]

Oh, that explicit documentation makes it hard to simply change this internal type 🙈

The documented behaviour is dodgy and inconsistent. It generally doesn't make much sense, per #16532 (comment).

I understand the main purpose of #stream is to send one message of unknown size. It handles automatic splitting into fragments for efficiency.
Manual fragment control can be an optional feature, but I don't see many use cases for it.

The documentation describes #flush very unexpectedly as sending the remaining data and resetting the stream. But that contradicts the primary intention of sending one message.
Calls to IO#flush are usually implicit and cannot be controlled from the outside. They are not expected to have any semantic effect. But resetting the stream IO is a significant difference in WebSocket communication: Instead of sending one single message, we're sending two (or more). It's unacceptable that this could happen involuntarily because of some call to #flush.
I don't think an implementation of IO#flush can be allowed to have such a grave impact. Such behaviour requires a more explicit method such as #reset.

On top of that, the implementation in StringIO#flush is buggy because it continues sending continuation fragments even after the final fragment of the previous message.

At this point, I would argue we can perhaps disregard the documentation of WebSocket#stream: It's inconsistent, violates the intention of IO#flush and hasn't even been implemented correctly.

We could implement StreamIO#flush but it should only send the current fragment, not end the entire message. This has no effect on message semantics, as expected of #flush.

[ysbaddaden]

@spuun I understand the issue. I'm proposing that #flush would merely send a frame with any pending data from the buffer and not send the FIN. I was wondering if sending an empty frame with FIN on close only (not flush) would be a problem.

[ysbaddaden]

We can just not implement #flush and remove the mentions in the HTTP::WebSocket#stream documentation.

Or we implement it as merely sending the buffer:

def flush : Nil
  send_data unless @pos == 0
end

I think both are fine. Implementing it might keep some of the current behavior, but I'm not sure there is much value or use cases.

[straight-shoota]

I was wondering if sending an empty frame with FIN on close only (not flush) would be a problem.

That's just an empty fin fragment which should be totally fine.

Implementing it might keep some of the current behavior, but I'm not sure there is much value or use cases.

I don't think implement StreamIO#flush has any valuable benefit. In standard WebSocket protocol, sending data in fragments makes no semantic difference. It waits until the entire message is received.
As mentioned in #16532 (comment) the only use case where you need control over the individual fragments would be protocol extensions. But that's a more complex scenario than StreamIO#flush could reasonably handle.

[ysbaddaden]

Then lets just remove the reference to #flush in the #stream method, and the PR is good to go?