Address review on stable-futures

[tomaka]

No description provided.

[mxinden]

Looks good to me. More details in the log might make it easier for newcomers.

[mxinden]

👍

[twittner]

I think we should return a Result from start_broadcast. A full channel is not the only error condition, the task may be gone for example. Also, the combination of start_broadcast and poll_ready_broadcast is racy if I am not mistaken.

[tomaka]

A full channel is not the only error condition, the task may be gone for example.

There are only two possibilities: a full channel, or the task gone.

A task being gone is a normal code path. It's totally possible for the task to close itself right after start_broadcast has returned, so this is racy anyway and returning an Ok cannot guarantee that the message was actually delivered.
If start_broadcast returned a Result, I wouldn't know what to do with that Result expect ignore it.

Also, the combination of start_broadcast and poll_ready_broadcast is racy if I am not mistaken.

For the "full channel" possibility, they can't be racy. They both take a &mut self, and it's therefore not possible for something to push a message between poll_ready_broadcast and start_broadcast.

[twittner]

Also, the combination of start_broadcast and poll_ready_broadcast is racy if I am not mistaken.

For the "full channel" possibility, they can't be racy. They both take a &mut self, and it's therefore not possible for something to push a message between poll_ready_broadcast and start_broadcast.

I think that an API can only be considered non-racy if the API can ensure that a sequence of operations is not interleaved by multiple threads (or rather that its correctness does not depend on it), but not when relying on the caller to establish this invariant. For example, this API

pub fn api(&mut self, ...) {
    self.a();
    self.b();
}

fn a(&mut self) { ... }
fn b(&mut self) { ... }

is non-rancy as the &mut self rules out interleavings, but

pub fn a(&mut self) { ... }
pub fn b(&mut self) { ... }

can not establish this invariant as mentioned here.

[tomaka]

I don't understand the reasoning.
To give a similar example, anyone could do this:

// thread 1 (t1: Arc<Mutex<Vec<T>>>):
if !t1.lock().is_empty() {
    t1.lock().remove(0);
}

// thread 2 (t2: Arc<Mutex<Vec<T>>>):
if !t2.lock().is_empty() {
    t2.lock().remove(0);
}

But that doesn't mean that Vec's API is racy.

[twittner]

Vec's API is not racy because it does not express a connection between the result of Vec::is_empty and Vec::remove. This PR does make a connection between start_broadcast and poll_ready_broadcast:

// Note that the user is expected to call poll_ready_broadcast beforehand,
// which returns Poll::Ready only if the channel isn't full. Reaching this
// path always indicates a mistake in the code.

It is similar to the Sink API in this regard.

[romanb]

From my understanding, what makes such an API racy, and the crucial difference to e.g. the Vec example, is the assumptions made in the implementation of such functions. An implementation of a() and b() where b() requires a() to be called first and its implementation relies on that fact (e.g. by ignoring errors caused by a possible interleaving) is racy. Vec::remove does not make an assumption that Vec::is_empty was called before - it just errors if the index is out of bounds (with a panic even). This is in contrast to a function like b() ignoring errors that "should not happen" based on the assumption that a() was just called. The situation is somewhat improved with logging (hence my request) but I would also still consider it a racy API if errors are not signaled from b().

Just my two cents.

EDIT: @twittner beat me to it. I basically wrote the same thing as the above.

[tomaka]

Returning an error raises another problem: since we send on the channel once for each node, what do we do if for example we successfully send the message to 5 nodes, but the 6th node's channel is full? We obviously can't cancel sending to the first five.

[twittner]

Yes, that is a problem. Maybe we could do with something like this:

pub fn broadcast<I: Clone>(&mut self, event: &I, cx: &mut Context) -> Poll<()> {
    for task in self.tasks.values_mut() {
        if task.sender.poll_ready(cx).is_not_ready() {
            return Poll::Pending
        }
    }
    for task in self.tasks.values_mut() {
        task.sender.start_send(...) ...
    }
    Poll::Ready(())
}

[tomaka]

I changed the PR completely.

[twittner]

👍