boostorg / beast

HTTP and WebSocket built on Boost.Asio in C++11
http://www.boost.org/libs/beast
Boost Software License 1.0
4.36k stars 634 forks source link

Change websocket stream read signature #410

Closed vinniefalco closed 7 years ago

vinniefalco commented 7 years ago

Instead of having opcode as an output reference parameter it could be a property of the stream, "last opcode" and accessible by a member function.

harrishancock commented 7 years ago

I'd love this change!

I have a notional MessageOrientedAsyncStream concept in my code. Types satisfy the concept requirements by implementing the following unsurprising interface:

struct X {
    template <class DynamicBuffer, class CompletionToken>
    auto async_read(DynamicBuffer&& b, CompletionToken&& t);
    // Read one single message into `b`.

    template <class ConstBufferSequence, class CompletionToken>
    auto async_write(const ConstBufferSequence& b, CompletionToken&& t);
    // Write `b` as one single message.

    // ...
};

Currently I need a little adapter class which provides the opcode parameter for me to make beast::websocket::stream fit the concept. This isn't terribly onerous, but a more generic async_read() would be welcome.

vinniefalco commented 7 years ago

Hey I just re-read this and I like this idea of AsyncMsgStream (see what I did there?).

harrishancock commented 7 years ago

Thanks! That's certainly less of a mouthful. I find the concept useful in my code: the two models of it I have are WebSockets and a homebrew HDLC-like framing wrapper around a serial_port. Certainly there are many more.

vinniefalco commented 7 years ago

Bikeshed moment:

/// Returns `true` if the last message frame indicated a binary opcode
bool stream::is_binary() const;

/// Returns `true` if the last message frame indicated a text opcode
bool stream::is_text() const
{
  return ! is_binary(); // derp
}

Note however that I am getting rid of that silly set_option and just making them member functions. So stream::binary(bool) will change the setting on outgoing messages, and bool stream::binary() const will return the setting for outgoing messages.

harrishancock commented 7 years ago

I think that's a good direction. The difference between my_stream.is_binary() and my_stream.binary() might be too subtle for some users -- it'd be fine with me, but I know other shed painters will weigh in during the review.

vinniefalco commented 7 years ago

Do you have any alternatives?

harrishancock commented 7 years ago

Not really. last_frame_was_binary()?

if (my_stream.opcode() == beast::websocket::opcode::binary) {
    // ...
}

if (my_stream.is_binary()) {
    //
}

if (my_stream.last_opcode() == beast::websocket::opcode::binary) {
    // ...
}

if (my_stream.last_frame_was_binary()) {
    // ...
}

Of those, I think my_stream.is_binary() reads the nicest.

vinniefalco commented 7 years ago

I'd like to get rid of opcode because it exposes protocol internals that the user can't see

harrishancock commented 7 years ago

I think that's wise. The first time I looked at the different values of the enum as a user, I was a little intimidated. It made using the protocol seem harder than it needed to be.

vinniefalco commented 7 years ago

How about

bool stream::got_text() const;

bool stream::got_binary() const
{
  return ! got_text();
}
harrishancock commented 7 years ago

I like it. It's clearer what it means to me than the is_binary() style.

vinniefalco commented 7 years ago

Synopsis:

void binary(bool); // set outgoing to binary
bool binary() const; // `true` if outgoing is binary

void text(bool); // set outgoing to text
bool text() const; // `true` if outgoing is text

bool got_binary() const; // last incoming was binary
bool got_text() const; // last incoming was text

This will go into v52

vinniefalco commented 7 years ago

See #446

vinniefalco commented 7 years ago

This is done