Consumer stall after RequeueWithoutBackoff

[jehiah]

If a consumer is in backoff and attempts to recover with RDY 1 and then a message, but response with RequeueWithoutBackoff it will stall and not exit backoff. (failing test attached)

cc: @georgicodes

[mreiferson]

🔥

[jehiah]

RFR @mreiferson

[mreiferson]

I'm not following the need for jehiah@f0be171

[jehiah]

I moved the lock so that it covered both the r.inBackoffBlock() check on line 648 and the updating of backoffDuration which is set on line 692. Without that spanning both i suspect there is a race condition in startStopContinueBackoff. (though to be honest i just noticed this reading the code; there could be other factors that keep that from not being an issue).

As i read it, that potential race could allow simultaneous finishes on more than one message/connection (this is fed by per-connection goroutines) to update rdy and backoff unexpectedly.

[mreiferson]

@jehiah that's atomic though? The only thing it needs to protect is backoffCounter... Let's remove that commit?

[jehiah]

Yes, backoffCounter updates need to be protected, but that update also needs to also include the check and update for inBackoffBlock or it will be possible to have it executed multiple times (sequentially) in the same backoff time period.

ie: pathalogical current case is you have 10 messages on different connections req to trigger backoff; they all pass the inBackoffBlock and then sequentially (because of the current lock) each update backoffCounter. We want only one of those to proceed past the inBackoffBlock check and update the counter. The first message that passes should prevent any other message (ie: by the lock also encompassing the update to backoffDuration).

[mreiferson]

Ahh, I thought you meant a "data" race.

Gonna read through again, thanks for the info...

[mreiferson]

OK - I think you're right, nice catch.

While I was reading the code I cleaned up some other related things (pushed here).

My suggestion is - can't we use atomic ops for backoffCounter? This would mean that backoffMtx is more clearly only used to serialize execution of startStopContinueBackoff.

[mreiferson]

(I'm going to give that a try)

[mreiferson]

@jehiah check those

[jehiah]

@mreiferson changes look good. two more small tweaks pushed. If you are good i'll squash this down.

[mreiferson]

👍 🔨

[mreiferson]

P.S. bump go versions in travis too?

[twmb]

Does backoff with a MaxBackoffDuration of 0 not race with resume?

The time.AfterFunc in backoff will immediately try to call resume, randomly picking a connection to updateRDY(1), while backoff is still finishing trying to updateRDY(0) for all connections.

I might be reading it wrong, though; I've not yet finished reading all of conn/consumer/delegates to see how it all fits together.

[mreiferson]

@twmb nice catch, I think you're right.

@jehiah we need to reorder those operations (RDY 0 should be sent before we set the timeout for resume() and we shouldn't ever send RDY 0 or timeout if the calculated duration is 0).

[twmb]

Not sending RDY 0 if backoff duration is 0 is another way of addressing #129.