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

newer
Re: [boost] [boost.cobalt] A...

older
[release] Master branch closes for...

Andrzej Krzemienski

9 Nov 2023 9 Nov '23

1:41 p.m.

...

From the perspective of the library author, you can now, for the event of

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-... Example: ``` promise<string> talk_to_server_1(); promise<string> talk_to_server_2(); promise<string> timeout_with_default(); vector servers = { talk_to_server_1(), talk_to_server_2(), timeout_with_default() }; pair ans = co_await race(servers); ``` This obviously cannot work when the input range passed to `race` is empty. Boost.COBALT docs say that for this situation "Selecting an empty range will cause an exception to be thrown". But I do not think it is the right approach. The right approach would be to clearly communicate to the user of the library that this is an incorrect usage of the library: if you pass an empty range to `race` you clearly do not know what you are doing, or you simply overlooked it and planted a bug. 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. 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. 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? Regards, &rzej;

Show replies by date

Klemens Morgenstern

9 Nov 9 Nov

2:09 p.m.

On Thu, Nov 9, 2023 at 9:41 PM Andrzej Krzemienski via Boost wrote:

...

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.

I don't see why this is a correctness issue and not a design choice. When is it correct to use an exception?

...

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-...

Example:

``` promise<string> talk_to_server_1(); promise<string> talk_to_server_2(); promise<string> timeout_with_default();

vector servers = { talk_to_server_1(), talk_to_server_2(), timeout_with_default() }; pair ans = co_await race(servers); ```

This obviously cannot work when the input range passed to `race` is empty.

This example doesn't help, because this should obviously use the variadic version. The reason to use the ranged one is for when you have another component dictating what elements to listen to. try { vector servers; for (const auto & server : config.get_servers()) servers.push_back(talk_to_server(server)); co_await race(servers); } catch(std::exception &) { // handle errors from running any server } That means the code using the ranged version is likely unaware of the actual value, because there's a very high likelihood it's not known at compile time.

Andrzej Krzemienski

3:01 p.m.

czw., 9 lis 2023 o 15:09 Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> napisał(a):

...

On Thu, Nov 9, 2023 at 9:41 PM Andrzej Krzemienski via Boost wrote:

...
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-...

...
Example:

``` promise<string> talk_to_server_1(); promise<string> talk_to_server_2(); promise<string> timeout_with_default();

vector servers = { talk_to_server_1(), talk_to_server_2(), timeout_with_default() }; pair ans = co_await race(servers); ```

This obviously cannot work when the input range passed to `race` is

empty.

This example doesn't help, because this should obviously use the variadic version.

The reason to use the ranged one is for when you have another component dictating what elements to listen to.

try { vector servers; for (const auto & server : config.get_servers()) servers.push_back(talk_to_server(server));

co_await race(servers); } catch(std::exception &) { // handle errors from running any server }

That means the code using the ranged version is likely unaware of the actual value, because there's a very high likelihood it's not known at compile time.

Thanks Klemens. This is very useful information (when to use which overload), and I recommend that it should be put in the documentation. Stil, I do not feel convinced. Your example illustrates, I think, the situation where an untrusted external input is passed unvalidated to the guts of the program. I would never like to encourage or support this way of writing servers. Of course, people will still do it, and it is prudent of you to be prepared for it, but I would still strongly insist that your library calls this usage a bug: that is something that the programmer should fix as soon as they can. It doesn't prevent you from throwing exceptions, but you have an opportunity to additionally discourage the use like this (via warnings, crashes in debug mode, additional control flows in debugger).

...

...
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.

I don't see why this is a correctness issue and not a design choice. When is it correct to use an exception?

To some extent, this is your choice as a library author. But I am trying to set up a guideline or a policy applicable to a range of libraries. The uncontroversial circumstance that warrants a throw, is when a function cannot deliver its promise, but the caller couldn't have known about it when calling a function: ``` File f ("C:/folder/filename.cfg"); ``` Here I checked that the file existed one millisecond ago. I requested for it to be open, but the OS determined that the file is no longer there. There was nothing I can do to guarantee that the file would be openable. The case with `race` is different. There is a very simple way of checking the empty-range, and it never can produce a meaningful result. Regards, &rzej;

Andrzej Krzemienski

4:28 p.m.

czw., 9 lis 2023 o 16:01 Andrzej Krzemienski napisał(a):

...

czw., 9 lis 2023 o 15:09 Klemens Morgenstern < klemensdavidmorgenstern@gmail.com> napisał(a):

...
On Thu, Nov 9, 2023 at 9:41 PM Andrzej Krzemienski via Boost wrote:

...
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-...

...
Example:

``` promise<string> talk_to_server_1(); promise<string> talk_to_server_2(); promise<string> timeout_with_default();

vector servers = { talk_to_server_1(), talk_to_server_2(), timeout_with_default() }; pair ans = co_await race(servers); ```

This obviously cannot work when the input range passed to `race` is

empty.

This example doesn't help, because this should obviously use the variadic version.

The reason to use the ranged one is for when you have another component dictating what elements to listen to.

try { vector servers; for (const auto & server : config.get_servers()) servers.push_back(talk_to_server(server));

co_await race(servers); } catch(std::exception &) { // handle errors from running any server }

That means the code using the ranged version is likely unaware of the actual value, because there's a very high likelihood it's not known at compile time.

Thanks Klemens. This is very useful information (when to use which overload), and I recommend that it should be put in the documentation.

Actually, your explanation convinces me even stronger that this should be a different function with a different name. It is a different use case, it has a different signature, a different return type. Regards, &rzej;

...

Stil, I do not feel convinced. Your example illustrates, I think, the situation where an untrusted external input is passed unvalidated to the guts of the program. I would never like to encourage or support this way of writing servers. Of course, people will still do it, and it is prudent of you to be prepared for it, but I would still strongly insist that your library calls this usage a bug: that is something that the programmer should fix as soon as they can. It doesn't prevent you from throwing exceptions, but you have an opportunity to additionally discourage the use like this (via warnings, crashes in debug mode, additional control flows in debugger).

...
...
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.

I don't see why this is a correctness issue and not a design choice. When is it correct to use an exception?

To some extent, this is your choice as a library author. But I am trying to set up a guideline or a policy applicable to a range of libraries. The uncontroversial circumstance that warrants a throw, is when a function cannot deliver its promise, but the caller couldn't have known about it when calling a function:

``` File f ("C:/folder/filename.cfg"); ``` Here I checked that the file existed one millisecond ago. I requested for it to be open, but the OS determined that the file is no longer there. There was nothing I can do to guarantee that the file would be openable.

The case with `race` is different. There is a very simple way of checking the empty-range, and it never can produce a meaningful result.

Regards, &rzej;

Julien Blanc

2:28 p.m.

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-...

<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 awaitable race(Promise && ... p); could be better specified as: template 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 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 awaitable left_race(PromiseRange && p); (2) template 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

Klemens Morgenstern

3:21 p.m.

On Thu, Nov 9, 2023 at 11:00 PM Julien Blanc via Boost wrote:

...

For the ranges overload:

template 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 awaitable left_race(PromiseRange && p);

How is `non_empty_range` defined?

Julien Blanc

4:22 p.m.

Le 2023-11-09 16:21, Klemens Morgenstern via Boost a écrit :

...

On Thu, Nov 9, 2023 at 11:00 PM Julien Blanc via Boost wrote:

...

...
(1) template awaitable left_race(PromiseRange && p);

How is `non_empty_range` defined?

range<awaitable> is already pseudo-code, so i'm not sure why you ask. But you're right, i've been mislead by the current doc. I was going to reply that it could at the very least be defined as: template<typename T> concept non_empty_range = std::ranges::range<T>; // self-documenting alias. Gives opportunity for future improvements However, this is not what we want here. What we want here is a non_empty range of<awaitable>. So i believe the correct documentation should be non_empty_range_of<awaitable>. Of course, it's still pseudo-code. This makes me rises the concern about using pseudo-code in the documentation, without telling so. Especially with features the reader might not be familiar with, such as concepts and coroutines, which . Maybe there should be a convention across the documentation to differentiate between valid c++ code and pseudo-code (pseudo-code is useful for clarity in certain situations). In all cases, i think that a misleading pseudo-code is a documentation bug that should be fixed. A library targetting c++20 should not misuse the "range" name. Best regards, Julien

383

Age (days ago)

383

Last active (days ago)

List overview

Download

6 comments

3 participants

participants (3)

Andrzej Krzemienski
Julien Blanc
Klemens Morgenstern