fix(producer): prevent deadlock in non-idempotent producer retryBatch - #3691
fix(producer): prevent deadlock in non-idempotent producer retryBatch#3691lbcjbb wants to merge 2 commits into
Conversation
598791d to
ba7eb60
Compare
…non-idempotent producers (IBM#3689) PR IBM#3644 changed `handleError()` to use `retryBatch()` for non-idempotent producers, keeping partitions muted during retries. However, `handleSuccess()` was not updated consistently: it called `retryBatch()` with `alreadyMuted=true`, but did not add the partition to `keepMuted`, causing the partition to be unmuted immediately after launching the retry goroutine. This inconsistency could lead to message reordering, as a new batch for the same partition could be sent before the retry completes. This commit aligns `handleSuccess()` with `handleError()` by: - Always populating `keepMuted` for retriable errors (not just for idempotent) - Always calling `RefreshMetadata()` before retries (not just for idempotent) Signed-off-by: Jean-Baptiste Bronisz <jean-baptiste.bronisz@adevinta.com>
…IBM#3689) PR IBM#3644 introduced a regression that can cause a `SyncProducer` to block indefinitely when using non-idempotent mode with connection retries. The deadlock scenario: 1. A connection error triggers `handleError()`, which launches a goroutine calling `retryBatch()` for the failed batch. 2. `retryBatch()` creates a new `brokerProducer` via `getBrokerProducer()` (refcount=1). 3. `retryBatch()` sends the batch on `bp.output`, then immediately releases its reference via defer `unrefBrokerProducer()` (refcount=0). 4. With `refcount=0`, the `brokerProducer` shuts down while the batch is still in flight waiting for a response from the broker. 5. The response is lost, `returnSuccesses()` / `returnErrors()` is never called. 6. `SendMessages()` blocks forever waiting on `ProducerMessage.expectation`. The fix stores the `brokerProducer` reference in the `produceSet` itself, so it is only released after `handleResponse()` has fully processed the batch (success or retry exhausted). This ensures the `brokerProducer` stays alive until the batch lifecycle is complete. Signed-off-by: Jean-Baptiste Bronisz <jean-baptiste.bronisz@adevinta.com>
ba7eb60 to
1143d11
Compare
|
Thanks @lbcjbb for the original issue report. Having dug into this, I don't believe the premature shutdown is actually the failure mode: when the retryBatch-created brokerProducer is un-reffed to zero, its shutdown() still drains and handles every pending response after closing output, so the retried batch's response isn't lost and the partition does get unmuted. The deadlock is that this unmute happens on a goroutine the stranded brokerProducer never hears about, because its run() loop had no case to wake on an unmute. That's what the other PR fixes. The handleSuccess changes here would also alter non-idempotent retry behaviour beyond the scope of the regression, so it's not obvious what issue they're be fixing or improving? |
|
Yes, I came to the same conclusion. I introduced a delay in the execution of
I've included these changes in this PR, but they are more complementary to PR #3644 than directly related to the regression. I'm going to move them to a dedicated PR (#3693). Thank you for your reply and for taking the time to look into this 🙏 |
PR #3644 introduced a regression that can cause a
SyncProducerto blockindefinitely when using non-idempotent mode with connection retries.
The deadlock scenario:
handleError(), which launches a goroutinecalling
retryBatch()for the failed batch.retryBatch()creates a newbrokerProducerviagetBrokerProducer()(refcount=1).retryBatch()sends the batch onbp.output, then immediately releases itsreference via defer
unrefBrokerProducer()(refcount=0).refcount=0, thebrokerProducershuts down while the batch is stillin flight waiting for a response from the broker.
returnSuccesses()/returnErrors()is never called.SendMessages()blocks forever waiting onProducerMessage.expectation.The fix stores the
brokerProducerreference in theproduceSetitself, so itis only released after
handleResponse()has fully processed the batch (successor retry exhausted). This ensures the
brokerProducerstays alive until thebatch lifecycle is complete.
Fixes #3689