[networking-ts] DynamicBuffer should be passed by non-const reference.

[vinnie...@gmail.com]

Currently, the draft at:

<http://cplusplus.github.io/networking-ts/draft.pdf>

prescribes that stream read algorithms operating on DynamicBuffer objects take the buffer by a forwarding reference. For example:

    template<class SyncReadStream, class DynamicBuffer>

    size_t read(SyncReadStream& stream, DynamicBuffer&& b);

I believe this interface is defective. The implementation of read() takes ownership of `b` via move construction, as can be seen in the reference implementation of networking-ts:

<https://github.com/chriskohlhoff/networking-ts-impl/blob/6bf74be013e104e2893de245bc3379a6f3f20d54/include/experimental/__net_ts/impl/read.hpp#L122>

I suspect the reason for this interface is to make the dynamic_string_buffer and dynamic_vector_buffer wrappers returned by dynamic_buffer() work. For example:

    std::string s;

    read(stream, dynamic_buffer(s));

The problem with this interface can be seen for the case where a user simply wants to read into their own instance of dynamic buffer:

    beast::multi_buffer b; // meets the requirements of DynamicBuffer
    read(stream, b);

This code will compile but do the wrong thing. The implementation takes ownership of b by move construction, leaving the caller with a moved-from object. After the read operation completes, the received data will be discarded when the scope of the newly move constructed object is exited. Clearly this is not the intention of the algorithm.

This code will also compile, and produce undesirable results:

    beast::multi_buffer b;
    read(stream, std::move(b));

There are two ways to make algorithms like read() work with dynamic buffers. The first is to provide special overloads for particular dynamic buffers. For example, asio::basic_streambuf meets the dynamic buffer requirements (actually it is the object that inspired the DynamicBuffer concept). But basic_streambuf cannot work with read algorithms which receive the DynamicBuffer by forwarding reference. For this reason, the net-ts compatible version of Asio provides overloads of read() to plug the hole created by this design flaw:

<https://github.com/chriskohlhoff/asio/blob/230c0d2ae035c5ce1292233fcab03cea0d341264/asio/include/asio/read.hpp#L414>


The other solution is to create a "dynamic buffer wrapper" which meets the requirements of DynamicBuffer and forwards its member function calls to an external object. In other words a non-owning reference. Example:

    template<class DynamicBuffer>
    struct dynamic_buffer_wrapper;

    template<class DynamicBuffer>
    dynamic_buffer_wrapper<DynamicBuffer>
    make_dynamic_buffer_wrapper(DynamicBuffer& buffer);

With these declarations a user can wrap their dynamic buffer at each call site to achieve the correct results:

    beast::multi_buffer b;
    read(stream, make_dynamic_buffer_wrapper(b));

This is not a novel idea, the net-ts compatible version of Asio creates such a wrapper internally, called basic_streambuf_ref to plug the hole created by this design flaw:

<https://github.com/chriskohlhoff/asio/blob/230c0d2ae035c5ce1292233fcab03cea0d341264/asio/include/asio/basic_streambuf.hpp#L362>

The wrapper approach is unsatisfying because it imposes an unnecessary notational burden at every call site, with little gain.

There is a larger problem lurking. Without explicitly stating it, the Networking-TS specification and accompanying reference implementation has managed to create two categories of dynamic buffer. One is the set of dynamic buffers which own their storage, and the other is the set of dynamic buffer wrappers which simply reference storage elsewhere and can be moved and destroyed without releasing the referenced storage.

The specification and implementation require that all dynamic buffers passed to net-ts algorithms are of the second variety. That is, that they do not own their storage. As such, they are fundamentally incompatible with existing practice such as the wide variety of dynamic buffers found in Boost.Beast:

Table 1.5 Dynamic Buffer Implementations
<http://www.boost.org/doc/libs/master/libs/beast/doc/html/beast/using_io/buffer_types.html>

There is a simple solution to this problem. That is to change the signature of Net-TS algorithms which receive dynamic buffer parameters to use regular non-const references. The signature for read, shown earlier, would be modified thusly:

    template<class SyncReadStream, class DynamicBuffer>

    size_t read(SyncReadStream& stream, DynamicBuffer& b);

With this signature, we can eliminate the MoveConstructible requirement from DynamicBuffer. However, callers who wish to use the dynamic_string_buffer and dynamic_vector_buffer non-owning wrappers would be slightly inconvenienced. They can no longer construct the wrapper directly in the argument list, since temporaries cannot bind to non-const references. Instead they must explicitly declare the wrapper, like this:

    std::string s;
    dynamic_string_buffer<std::string> b;
    read(socket, b);

For the asynchronous case, the caller will have to manage the lifetime of the dynamic buffer wrapper. Some may feel that this is an unnecessary burden. I speculate that the wrappers were written to "make the networking-ts more approachable for novices." I feel that this is not a valid justification given the problems that it introduces, for two reasons.

First, in the synchronous case the caller already has the stack available. The transient wrapper can be placed there.

Second, for the asynchronous case, the caller already has to manage the lifetime of the wrapped object such as the string or vector. Requiring that they also manage the lifetime of the transient dynamic buffer wrapper is not a significant inconvenience.

My goal in making this post is to inform the networking-ts stakeholders of the problem. It is causing particular distress for me because I am trying to port Beast (<https://github.com/boostorg/beast>) to net-ts Asio which will appear in Boost 1.66.0 and it changes all of the function signatures. It is also unsightly for users - I have to now explain to them the difference between the two types of dynamic buffers (owning and non-owning) because the Net-TS specification does not explain it. And the users must be informed of the need to wrap their dynamic buffers at every single call site.

I would like to hear from anyone with some knowledge about net-ts, and especially the folks who participated in the meetings which led to these changes. I will also try to write a paper to address it for the next meeting.

Thanks

[T. C.]

To start with, that's a static_cast<DynamicBuffer&&> aka forward, see https://github.com/chriskohlhoff/networking-ts-impl/blob/master/include/experimental/__net_ts/detail/config.hpp#L122, although

in this case due to the SFINAE constraint it will be a move. That said, the specification of read also doesn't say if it moves from b, which strikes me as a problem.

Now, the DynamicBuffer's version of read only accepts rvalues, because is_dynamic_buffer<T> is never (or should never be) true for a reference type T. If this example of yours actually compiles, that would indicate a bug in the implementation.

beast::multi_buffer b; // meets the requirements of DynamicBuffer
read(stream, b);

I don't have an opinion on whether these functions should support owning dynamic buffers - considering that every other "buffer" in the TS is nonowning, I have serious doubts about whether the original design ever contemplated such things. If the intent is that "buffer"s are always nonowning, then "owning dynamic buffer" is nothing but a self-contradiction.

Nonetheless, if we want to support it, it seems to me that the minimal change necessary to support this in the specification is simply to relax the condition to is_dynamic_buffer<remove_reference_t<DynamicBuffer>>, and clarify that a move is either not performed at all or performed only for rvalues. The latter seems preferable to maintain consistency with the asynchronous operations, which presumably have to perform a move.

I see no reason why we need to break rvalue wrappers; that just trades one set of "existing practice" to break for another, and I suspect that that set of existing practice dates back to well before Beast.

[T. C.]

On Sunday, September 17, 2017 at 3:59:19 AM UTC-4, T. C. wrote:

I see no reason why we need to break rvalue wrappers; that just trades one set of "existing practice" to break for another, and I suspect that that set of existing practice dates back to well before Beast.

So I somehow got confused into thinking that DynamicBuffer has a longer pedigree than it does. Mea culpa.

Nonetheless, I'm still not seeing why we need to reject rvalue wrappers in order to support owning dynamic buffers. 

[T. C.]

On Sunday, September 17, 2017 at 3:59:19 AM UTC-4, T. C. wrote:

That said, the specification of read also doesn't say if it moves from b, which strikes me as a problem.

Actually, this might be allowed by [res.on.arguments]/1.3:

If a function argument binds to an rvalue reference parameter, the implementation may assume that this parameter is a unique reference to this argument.

If b is a unique reference, whether it is moved from may be considered unobservable.

Regardless, the spec could use some clarification here.