[client]: Fix for incorrectly dropped consensus messages (#11082)

[dforsten]

Fixes a race condition causing the currently_queued counter to underflow and consensus messages getting dropped incorrectly as a consequence.

The initial solution suggested in #11082 was dropped in favor of converting the atomic counter to a signed integer for robustness. There is no definite guarantee that if the send() message returns an error the message will not get executed at a later point, which would cause another potential underflow.

The fix requires a slight limitation of the maximum size of the queue, a 63bit positive integer range should however be well large enough for its purposes.

[parity-cla-bot]

It looks like @dforsten signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[dvdplm]

Did you consider changing the input arg to isize rather than casting it? Seems like there are 3 call sites in total (in Client::new()) so perhaps changing the type is not too bad?

[dforsten]

Changing the input arg type does spread into a couple of corners in the code, especially into client settings.
It is an option of course - on the other hand it should only be configurable as a positive number, making the setting itself a signed integer might be confusing to the user.
Using a signed integer in IoChannelQueue is a local implementation strategy to ensure the queue size counter checks work correctly in a multi-threaded environment.

[dvdplm]

on the other hand it should only be configurable as a positive number, making the setting itself a signed integer might be confusing to the user.

That could be handled when reading the user provided config, i.e. continue using an unsigned integer in configs and convert to signed there rather than for each message.

[dvdplm]

Not your code, but maybe this can be a private method?

[ordian]

There is no definite guarantee that if the send() message returns an error the message will not get executed at a later point, which would cause another potential underflow.

Could you elaborate how's that possible? I thought if send(msg) returns an error, it means the msg won't be executed.

Using a signed integer is a clever trick, but seems more like a hack than a proper solution to me.

[ordian]

The initial solution suggested in #11082 was dropped in favor of converting the atomic counter to a signed integer for robustness.

Have you been able to reproduce the issue with the initial solution?

[dforsten]

There is no definite guarantee that if the send() message returns an error the message will not get executed at a later point, which would cause another potential underflow.

Could you elaborate how's that possible? I thought if send(msg) returns an error, it means the msg won't be executed.

Reading the implementation of send(msg) I am not at all certain that if it returns an error the message will not be executed. If there is an absolute guarantee that this is the case then we can implement the solution I originally suggested.

[dvdplm]

Reading the implementation of send(msg) I am not at all certain that if it returns an error the message will not be executed. If there is an absolute guarantee that this is the case then we can implement the solution I originally suggested.

Looking at the send() impl it seems like one of the two code paths – send_sync() – never returns an error but even in that case, e.g. a failed call to Weak::upgrade(), would not send the message.
Do you know which of the two code paths you end up in when you've encountered the problem?

In other words: I think your original suggestion was the right one.

[dforsten]

The code section I am most unsure if it will return an error even if the message will eventually be sent is this one in channel.rs of mio:

    pub fn try_send(&self, t: T) -> Result<(), TrySendError<T>> {
        self.tx.try_send(t)
            .map_err(From::from)
            .and_then(|_| {
                self.ctl.inc()?;
                Ok(())
            })
    }

try_send() is ok, but I am unclear if when self.ctl.inc()?; fails the message actually gets de-queued or sent at a later point.

[dvdplm]

I am unclear if when self.ctl.inc()?; fails the message actually gets de-queued or sent at a later point.

Help me check if I understand this right:

Afaict if try_send() succeeds then the message is sent; if self.ctl.inc()?; fails after that we'd see an error bubble back up to the caller and we'd have a situation where the closure returns an Err but the message was actually sent (so we'd see a decrement). In that scenario we'd bail before doing the increment.
Did I get that right? There is no de-queueing and/or re-trying happening though, or am I missing something?

When self.ctl.inc()?; fails it returns a std::io::Error so it's not something we can match on I think. If it returned a more specific error variant we could perhaps look for that specifically.

I think I understand what you mean by the original solution not being the right one: we'd increment before sending, send the message and still get an error back. Now we'd do two decrements (one from the message closure and the other in the Err => branch in the caller) and could underflow.

Sounds like your second solution with the signed int is the least worst. :/

[dvdplm]

Sounds like your second solution with the signed int is the least worst. :/

With this PR we could end up with negative currently_queued: maybe queue() should check for that before sending and reset to 0 if that is the case?

[ordian]

Long-term we want to rewrite the whole mio-thing, and since we're not confident try_send won't execute the message on error, it seems like the simplest workaround atm.

[ordian]

Please add docs why we're using signed integers here, so it won't get accidentally reverted in the future.

[dforsten]

I added documentation to the currently_queued field to clarify why signed integers are being used here.

[dvdplm]

Long-term we want to rewrite the whole mio-thing,

Indeed. And on that note it's a bit weird to send in an "item count" from the outside to the queue. The queue itself should keep count of how many messages it has pending/sent/failed etc. The only callsite where count is not 1 is when we enqueue transactions.

[dforsten]

Sounds like your second solution with the signed int is the least worst. :/

With this PR we could end up with negative currently_queued: maybe queue() should check for that before sending and reset to 0 if that is the case?

The only effect a negative number would have is that the queue size limit would effectively become larger. Before this fix it would straight away fail in that case, and not forward consensus messages at all.

[dvdplm]

The only effect a negative number would have is that the queue size limit would effectively become larger. Before this fix it would straight away fail in that case, and not forward consensus messages at all.

Yes, I am clear on that. It'd still be good to avoid negative numbers I think. If you disagree, do you mind explaining why?

[ordian]

Yes, I am clear on that. It'd still be good to avoid negative numbers I think. If you disagree, do you mind explaining why?

Negative numbers could appear only temporarily due to race condition, and if we reset them to zero, currently_queued will be incorrectly indefinitely growing and reach the limit at some point.

[ordian]

There is no definite guarantee that if the send() message returns an error the message will not get executed at a later point, which would cause another potential underflow.

But with this in mind, both the old and the signed implementation are still incorrect and could underflow.

[dforsten]

There is no definite guarantee that if the send() message returns an error the message will not get executed at a later point, which would cause another potential underflow.

But with this in mind, both the old and the signed implementation are still incorrect and could underflow.

That is correct, if send() returns an Error without dropping the message the counter will decrement by one each time. With unsigned numbers the effect is an immediate underflow, with 64bit signed numbers it will take a very long time until it underflows.

If that case happens a lot or constantly the validator node should be considered faulty though since consensus messages will likely take a long/indefinite time to be processed.

[dforsten]

Is there still anything I should change on my end to get this PR merged?

[dvdplm]

It's kind of a hack, but it is (much) better than what we have.

Thank you for taking the time, much appreciated.