Add methods on WebSocket to better implement Sink::poll_ready

[coolreader18]

I noticed in tokio-tungstenite that Sink isn't quite implemented as it should be - poll_ready actually flushes the sink, and start_send has the possibility to do some IO. This adds methods, write_pending_ready and submit_message that can be used to implement those more correctly.

I wasn't sure whether to have write_pending_ready check for AlreadyClosed and SendAfterClosing, but went with it because I feel like that's what poll_ready would want. Also I'm incredibly unsure about naming for these, please suggest better ones (especially write_pending_ready).

[daniel-abramov]

That looks like an interesting idea. I'll try to review it in more detail and check for regressions over the holidays.

As for the naming, I think instead of submit_message() we could use enqueue_message().

Not quite sure about write_pending_ready(). I think our public interface of the WebSocket should potentially be able to cover all the use cases with a combination of:

• write_message() - usual semantics.
• write_pending() - usual semantics.
• read_message() - usual semantics.
• enqueue_message() - tries adding a message to the queue without writing it (for the use with tokio-tungstenite and futures).

[coolreader18]

write_pending() for poll_ready works, but it's not as efficient as it could be - the Sink trait is designed with buffered message queues in mind, so if it's waiting for IO-readiness in order to flush the queue before every send, that's unnecessary waiting, when it could be (and tungstenite already supports this!) buffered and later flushed.
(also sorry for the late response)

[daniel-abramov]

That's right.

What I mean is that we need at least the following functions to fully cope with the semantics in tokio-tungstenite's Sink:

1. Enqueue a message (optionally without actually doing any I/O, strictly speaking start_send() may try to do flushing to make room for a message, but it should not flush the whole queue as it would be against the semantics) a.k.a. start_send().
2. Make room for a single message a.k.a. poll_ready().
3. Flush the whole queue a.k.a. poll_complete().

Currently, tungstenite only has write_message() and write_pending(), where the first one tries to make room for a message, add a message to the queue and flush the queue (too many things for a single function). And write_pending() that flushes the queue. Additionally, we call write_pending() when reading.

Ideally we want to split them so that we have separate methods for each of these stages to utilise it from tokio-tungstenite efficiently.

[daniel-abramov]

If I copy you right, write_pending_ready() is a function that is meant to be called from poll_ready()? - If that's so, then I'm afraid it won't work the way you anticipated, since it calls write_pending() inside which means that if the queue is full at the moment of calling it, it will flush the queue completely (!) before returning Ok(()), whereas the expected behaviour for poll_ready() would be to make a room for a single item (so that start_send() succeeds). This is essentially the main difference between poll_ready() and poll_complete() - the first one only flushes a single item, while the later flushes the whole sink. Does it make any sense?

[daniel-abramov]

poll_ready actually flushes the sink, and start_send has the possibility to do some IO.

Totally agree on poll_ready() (we lack a function that represents this behavior). But I'm not sure that start_send() is forbidden to do the I/O. While I agree that the most expected and efficient way would be for start_send() to only enqueue a message, according to the documentation it seems like it's not forbidden for start_send() to at least try to make room for a message if the queue is full:

Implementations of poll_ready and start_send will usually involve flushing behind the scenes in order to make room for new messages.

[alexheretic]

This looks somewhat related to the PRs I raised recently. I propose we

• Remove implicit flushing Rework to remove implicit flushes #357
• Remove flush on poll_ready Remove flush on each WebSocketStream::poll_ready call + update to tungstenite 0.20 tokio-tungstenite#284

[daniel-abramov]

Indeed, then I think I'll close it.