cplusplus / networking-ts Goto Github PK

Insert manual breaks or hyphenation

C++Std [reentrancy] says:

Except where explicitly specified in this standard, it is implementation-defined which functions in the Standard C++ library may be recursively reentered.

In the executor requirements, the intention is that the dispatch() function may be recursively reentered. A statement to this effect may need to be added to the requirements. (All dispatch() member functions provided by executors in the TS itself should then by implication allow reentrancy.)

As a613fa9 points out, the postconditions for the basic_socket_acceptor move ctor don't have any postconditions on native_handle().

To put it simply, the C++ specification is deficient when it comes to defining iterator concepts, because it conflates the traversal and value access concerns. This is fully described here:
http://www.boost.org/doc/libs/1_62_0/libs/iterator/doc/new-iter-concepts.html

I propose that the networking proposal break new ground and push for addressing this defect. Specifically, for the ConstBufferSequence and MutableBufferSequence type requirements, replace the BidirectionalIterator requirement on const_iterator with the dual requirements ReadableIterator and BidirectionalTraversalIterator, defined here:
http://www.boost.org/doc/libs/1_62_0/libs/iterator/doc/new-iter-concepts.html#readable-iterators-lib-readable-iterators

Rationale: This allows more flexible operations on buffer sequences, such as applying range transformations to the value type. Without this change, implementations that transform buffer sequences (such as concatenating them, adjusting the byte range they represent, or converting from mutable to const sequences) become overly complex, inefficient, or impossible.

Here are examples of buffer sequence transformations in use today which would be disallowed by the current BidirectionalIterator requirements in the networking TS:

https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/buffer_cat.hpp#L22

https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/buffers_adapter.hpp#L17

https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/consuming_buffers.hpp#L20

https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/prepare_buffers.hpp#L135

One implementation above, buffer_cat, is particularly useful as it allows multiple system calls (to read or write on a socket) to be reduced to one. Consider a network protocol that sends objects which have multiple parts, such as a message header followed by a message body. The header and body could each be represented by a ConstBufferSequence. buffer_cat allows sending the entire message in a single system call instead of doing it in two separate calls. This has been used in the wild to achieve impressive decreases in latency for a HTTP server. The current implementation of buffer_cat is rather elegant, it uses the equivalent of a std::variant to efficiently transform the inputs without making a copy of the sequences:
https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/detail/buffer_cat.hpp#L22

This implementation is unfortunately non conforming because the iterators it defines, do not meet the requirements of BidirectionalIterator since they return an rvalue:
https://github.com/vinniefalco/Beast/blob/70b8555cdca69b9c5777db02115d30d1c1aad631/include/beast/core/detail/buffer_cat.hpp#L103
This was done because buffer_cat allows conversion from mutable sequences to const sequences. For example, if a and b are **ConstBufferSequence* objects, and c is a MutableBufferSequence, the expression buffer_cat(a, b, c) returns an object representing the concatenation of a, b, and c, whose type meets the requirements of ConstBufferSequence.

The alternative to the elegant, current implementation of buffer_cat is one that uses a std::vector equivalent to store a precomputed version of the concatenated sequence. Its true that a specialization could make this vector implementation required in only a subset of use cases but the solution is non-ideal.

For this reason, I urge the authors to consider requiring only ReadableIterator and BidirectionalTraversalIterator for iterators representing buffer sequences.

maybe conformance.posix

See first change in cplusplus/draft#759 and also cplusplus/draft#763 (and pending improvements to those new environments from @tkoeppe that are likely to come soon)

According to TS, a type meets the ExecutionContext requirements if (among other things) it has a member function get_executor that returns an Executor. The type execution_context does not have such a member. This is very counter-intuitive.

const_buffer [buffer.const] is a non-owning type that consists of a pointer and a size.

Consider renaming const_buffer to buffer_view partly to indicate that users must keep the underlying, owning buffer alive during operations that involve const_buffer, and partly for naming consistency with string_view.

In the same vein, it would make sense to rename mutable_buffer [buffer.mutable] to something like buffer_span. While there is no naming precedence for this in C++17, there are various span proposals (e.g. P0122R1 and P0123R1.)

Maybe even 1-9

The Effects: element says "if the function makes a putback position available" but doesn't say how that might be achieved. Should it say "if the function can make a putback position available" ?

It seems strange for the description of the effects to say "if part of the effects happens" without saying how it happens.

See also cplusplus/draft#1552

Both the run() and run_one() functions include the following statement:

Must not be called from a thread that is currently calling a run function.

This restriction was originally added as a way to prevent users from entering a kind of "deadlock". This is because run() and run_one() can block until the io_context runs out of work. Since outstanding work includes currently executing function objects, if a function object makes a nested call to run()/run_one() that nested call could block forever as the work count can never reach zero.

However, it has been brought to my attention by users that there are valid use cases for making these nested calls. Deadlock can be avoided if some other condition will cause run()/run_one() to exit (e.g. an exception, explicit call to stop, run_one finished running a single function, etc). This condition can be known ahead of time by the user.

The existing implementation in asio does not make any beneficial use of this restriction.

Therefore, I propose striking those sentences from both those places. It is the responsibility of the user to avoid the conditions for deadlock.

binds an executor [...] to an object or function of type T.

Does this really mean to talk about functions? Uses-executor construction is only defined for object types, not function types.

Does it mean function objects? (Which are also objects).

Need to mark every library name and concept with \indexlibrary

I can't find any mention of SSL or TLS in the draft?

Why aren't SSL or TLS mentioned in the draft?

Maybe tie it more clearly to the previous subclause, or when introducing Executor1 say "In the specifications that follow, let Executor1 be ..."

Is it intended that users can provide overloads of buffer_size for user-defined buffer sequence types? Is it something we should consider? I'm thinking about basic_streambuf and user defined alternatives, it can compute buffer_size for its input and output sequences in constant time.

C++Std [reentrancy] says:

Except where explicitly specified in this standard, it is implementation-defined which functions in the Standard C++ library may be recursively reentered.

The intention is that both use_service and make_service may make nested calls (from the Service constructor) to use_service or make_service. Obviously these nested calls will require a different Service template argument. I am uncertain if calling these function templates with different template arguments counts as recursive reentrance, but if it does then we may need to add a sentence explicitly specifying that this is permitted.

C++Std [reentrancy] says:

Except where explicitly specified in this standard, it is implementation-defined which functions in the Standard C++ library may be recursively reentered.

The intention is that the run functions may be recursively reentered. We may want add a sentence explicitly specifying this.

[buffer.reqmts.mutablebuffersequence], [buffer.reqmts.constbuffersequence], [buffer.seq.access]

We should consider:

• Adding "Shall not exit via an exception." to the the requirements for net::buffer_sequence_begin(x) and 'net::buffer_sequence_end(x).
• Requiring that the conversion of the iterator value type to const_buffer or mutable_buffer should not exit via exception.
• (And perhaps place the same requirement on the iterator traversal and dereference too, although I'm not sure if this is already implied elsewhere in the standard?)
• Adding noexcept to the buffer sequence access functions.

The current implementation in asio assumes that these never throw, and in general I think low level buffer operations should not throw.

ISO directives do not allow hanging paragraphs, e.g.

8 Method of description (Informative) [description]

-1- This subclause describes the conventions ...

8.1 Structure of each clause [structure]

8.1.1 Detailed specifications [structure.specifications]

-1- In addition to ...

[description] p1 is "hanging". There needs to be a new subclause introduced for that paragraph.

The following Clauses and subclauses have hanging paragraphs:

8, 13.2.7, 13.5, 13.7, 13.12, 13.14, 13.16, 13.18, 13.21, 13.25, 13.26, 14.2, 14.3, 15, 15.5, 18.5, 18.6, 18.7, 18.8, 18.9, 19.1, 19.2, 21.4, 21.5, 21.6, 21.11, 21.12, 21.13, 21.14, 21.15, 21.17, 21.19, 21.20, 21.21

This is probably a copy/paste error introduced when this text was copied from the standard (I believe from [allocator.requirements]).

The intent is only that copy and move constructors shall not throw, and I think that is already covered by "copy operation, move operation". Other constructors not covered by the ProtoAllocator requirements should be allowed to throw.

Suggested resolution: Delete "constructor," so that the sentence reads "No comparison operator, copy operation, move operation, or swap operation on these types shall exit via an exception."

I don't see null_buffers in the TS:
http://www.boost.org/doc/libs/1_62_0/doc/html/boost_asio/overview/core/reactor.html

Is it there?

For v2 (or when merging into the IS) we should consider using std::byte instead of unsigned char.

Current table layout leaves a lot to be desired.

Each definition should be substitutable for the term where it occurs.

Is the networking TS dead ?

This runs into the right margin:

Remarks: length_error if the condition protocol().family() == AF_INET6 && s != sizeof(sock- addr_in6) || protocol().family() == AF_INET4 && s != sizeof(sockaddr_in) is true.

It could be changed to:

Remarks: length_error if either of the following conditions is true:
— protocol().family() == AF_INET6 && s != sizeof(sockaddr_in6),
— protocol().family() == AF_INET4 && s != sizeof(sockaddr_in).

But maybe it could be:

Remarks: length_error if s != size().

This isn't identical in the case where protocol.family() is neither AF_INET4 not AF_INET6 but that should be disallowed by the constructors.

[internet.address.v4.creation]/6 says (about make_address_v4):

Returns: If successful, an address_v4 value corresponding to the string str. Otherwise address_v4().

What is the benefit for returning all zeros?
Why not just say something like "Otherwise, the return value is unspecified".

When I'm parsing these bits, if I get a failure, I have to either (1) zero the internal array, or (2) return a different value, which defeats some cases of RVO.
(Obviously, this is not specific to address_v4]

The derived socket types basic_datagram_socket and basic_stream_socket should specify that their native_handle_type is the same as basic_socket::native_handle_type

The Executor requirements table and [async.system.exec.ops] both say:

Creates an object f1 initialized with DECAY_COPY(forward<Func>(f))

This performs an unnecessary copy, as DECAY_COPY already returns a new object, so this relies on elision to avoid two new objects being created. It also doesn't say what the type of f1 is, so calling f1() could do something entirely unrelated to f.

This seems simpler and more precise:

Creates an object f1 of type decay_t<Func>, initialized with forward<Func>(f).

The base standard is C++14, so 'is_base_of_v` doesn't exist.

Instead of repeating "if F == AF_INET6" several times, we could introduce a new variable:

and let F be the value of p.family()and let v6 be the value of p.family() == AF_INET6.

Then just use "if v6" in the table.

There should be a list, and the abbreviations (p2) probably should be in normative text rather than a note.

In TS ASIO's io_service was split into 2 things: ExecutionContext and Executor. Because of that, a type that meets Service requirements have no way of accessing Executor, instead it can only access its execution_context. Was this intentional?

When using socket options that are not defined by [socket.opt], users have to create their own class that follows the GettableSocketOption [socket.reqmts.gettablesocketoption] or SettableSocketOption [socket.reqmts.settablesocketoption] concepts.

As the majority of socket options are integral, it would ease the burden of using such socket options if an integer_option helper class was available (basically the same as asio::detail::socket_option::integer.) Its use would boil down to:

using maximum_segment_size_type = net::integer_option<IPPROTO_TCP, TCP_MAXSEG>;
maximum_segment_size_type mtu;
socket.get_option(mtu);

A similar helper class for boolean socket options could also be added.

This is probably a copy/paste error introduced when this text was copied from the standard (I believe from [allocator.requirements]).

The intent is only that copy and move constructors shall not throw, and I think that is already covered by "copy operation, move operation". Other constructors not covered by the Executor requirements should be allowed to throw.

Suggested resolution: Delete "constructor," so that the sentence reads "No comparison operator, copy operation, move operation, swap operation, or member functions \tcode{context}, \tcode{on_work_started}, and \tcode{on_work_finished} on these types shall exit via an exception."

c.f. cplusplus/draft#1497