Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address review on stable-futures #1373

Merged
merged 4 commits into from
Jan 6, 2020

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Jan 6, 2020

No description provided.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

core/src/nodes/tasks/manager.rs Outdated Show resolved Hide resolved
Co-Authored-By: Max Inden <mail@max-inden.de>
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

tomaka commented Jan 6, 2020

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
Copy link
Contributor

twittner commented Jan 6, 2020

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
Copy link
Member Author

tomaka commented Jan 6, 2020

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
Copy link
Contributor

twittner commented Jan 6, 2020

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
Copy link
Contributor

romanb commented Jan 6, 2020

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
Copy link
Member Author

tomaka commented Jan 6, 2020

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
Copy link
Contributor

twittner commented Jan 6, 2020

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
Copy link
Member Author

tomaka commented Jan 6, 2020

I changed the PR completely.

@tomaka tomaka requested a review from twittner January 6, 2020 16:21
Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tomaka tomaka merged commit ced66a3 into libp2p:stable-futures Jan 6, 2020
@tomaka tomaka deleted the loggin-stable-fut branch January 6, 2020 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants