Re: [boost.cobalt] A precondition or an error condition?

From: Julien Blanc (julien.blanc_at_[hidden])
Date: 2023-11-09 14:28:28

Le 2023-11-09 14:41, Andrzej Krzemienski via Boost a écrit :
> Hi Everyone,
> I would like to discuss one correctness aspect of Boost.COBALT. We have
> this list and the Slack channel, so I am not sure which place is 
> better.
> Let me do it here.
> 
> In Boost.Cobalt (formerly Boost.Async) we have function cobalt::race
> (formerly select()) that takes a range of awaitables `p` and returns an
> awaitable capable on awaiting on one of the elements of `p`:
> 
> https://www.boost.org/doc/libs/develop/libs/cobalt/doc/html/index.html#race-outline
> 
> <snip>
> 
> The library should list it as a *precondition* in the docs: the passed
> range cannot be empty. If you do it, you cannot expect your program to 
> work
> correctly.
> 
> From the perspective of the library author, you can now, for the event 
> of
> passing the empty range, putt all the tools that help in debugging:
> * BOOST_ASSERT
> * insert a breakpoint
> 
> And on the next line, you can also throw an exception to avoid an UB. 
> But
> in that case the exception would not be part of the official contract.
> 
> If the docs just say "throws an exception" (as today, which I think is
> wrong) then the library author is not allowed to insert any debugging 
> aids
> (such as BOOST_ASSERT or breaking into the debugger) because the user 
> may
> be relying on the exception throw.

I agree with you. However, there are pros and cons of both approaches 
(UB for contract violation, or broader contract and defined behaviour 
with exception thrown). In all cases, "throw an exception" is not a 
sufficient specification. The contract must clearly state which 
exception is thrown.

This is like the good old vector/array operator[] vs at(). There are use 
cases for both.

> You do not want the users to rely on the exception throw: you want to
> prevent the users from passing the empty range.
> 
> What do others think about it?

I think that as much as possible should be done using the type system 
and the compiler, because this is what is checked at compile time. In 
that particular case, i think that:

template<asio::cancellation_type Ct = asio::cancellation_type::all, 
awaitable... Promise>
awaitable race(Promise && ... p);

could be better specified as:

template<asio::cancellation_type Ct = asio::cancellation_type::all, 
awaitable Promise, awaitable... Promises>
awaitable race(Promise&& p1, Promises&& ... ps);

since here, we enforce at compile time that there is at least one 
Promise, so no exception is needed.

For the ranges overload:

template<asio::cancellation_type Ct = asio::cancellation_type::all, 
range<awaitable> PromiseRange>
awaitable left_race(PromiseRange && p);

It's fine as-is if it specifies which exception is thrown. Otherwise, 
i'd recommend either of the following:

(1) template<asio::cancellation_type Ct = asio::cancellation_type::all, 
non_empty_range<awaitable> PromiseRange>
awaitable left_race(PromiseRange && p);

(2) template<asio::cancellation_type Ct = asio::cancellation_type::all, 
range<awaitable> NonEmptyPromiseRange>
awaitable left_race(NonEmptyPromiseRange && p);

(1) ressembles the gsl::not_null. However, it may be non-trivial to make 
it really usable in practice (i have not yet played enough with concepts 
to know if something like that can be done, and to which point it can be 
compile-time enforced). (2) is a minor change that makes the requirement 
a lot more explicit and the code self-documenting.

Regards,

Julien