Deprecate Channel.empty/consumeOne

[sarneaud]

First off, here's a simple way to consume a channel that's correct (at least at the API level):

int i;
while (ch.tryConsumeOne(i))
{
    writeln(i);
}

The API also has consumeOne() and empty(). I guess this is the obvious way to use them:

while (!ch.empty)
{
    auto i = ch.consumeOne();
    writeln(i);
}

There's an obvious TOCTOU between the empty() and the consumeOne() with multiple consumers, as warned by the docs for empty():

Note that relying on the return value to determine whether another element can be read is only safe in a single-reader scenario. Use tryConsumeOne in a multiple-reader scenario instead.

But there's still a TOCTOU in the single-reader case because empty() doesn't block if there's nothing in the channel but the channel hasn't been closed yet. Here's a PoC:

import std.stdio;
import vibe.core.channel;
import vibe.core.core;
import core.time;

void producer(Channel!int ch)
{
    sleep(1.seconds);
    ch.close();
}

void consumer(Channel!int ch)
{
    while (!ch.empty)
    {
        auto i = ch.consumeOne();
        writeln(i);
    }
}

void main()
{
    auto ch = createChannel!int();
    auto p = runTask(&producer, ch);
    auto c = runTask(&consumer, ch);

    p.join();
    c.join();
}

This crashes with

Task terminated with uncaught exception: Attempt to consume from an empty channel.

I guess empty() could be changed to a blocking call, but honestly I think it's best to just deprecate it and consumeOne(). As long as empty() exists, people will be confused into using it like for Phobos ranges, but while(tryConsumeOne(x)) is simpler and better. consumeOne() is only correct in special cases when something like this is better: enforce(tryConsumeOne(x), "Protocol XYZZY requires a single x here")

[s-ludwig]

I stumbled over this when adding close, but didn't make a decision about the old API at the time (which I really should have, since it was still just in a branch). Today I would agree that tryConsumeOne and consumeAll should be the only way.

As a first step though, I would still opt to make empty blocking to make the single-reader scenario sound. I still haven't decided whether deprecating symbols warrants a major version bump or not, although I'm leaning towards allowing deprecations on minor versions for practicality purposes. Depending on that, the functions could then either be deprecated, or documented as "scheduled for deprecation".

[s-ludwig]

Opened a PR for fixing empty: #163