Conversation
…mmit With Consumer.Offsets.AutoCommit.Enable set to false, Close on a PartitionOffsetManager waited for its errors channel to be closed, but only the parent's Commit or Close releases a closed POM, and without auto-commit the parent has no loop calling Commit. Closing the POM before the OffsetManager, as the OffsetManager.Close doc asks, blocked until something else called Commit or Close on the OffsetManager. Release the POM from its parent in Close when auto-commit is disabled. Like removePartitions and OffsetManager.Close in this mode, it does not commit the marked offset, which remains the caller's job via Commit. The release runs in another goroutine while Close drains the errors, because a concurrent Commit may be blocked sending an error while holding pomsLock. It only releases the POM if it still manages its partition, so closing it twice can't release a newer POM for the same partition. Fixes IBM#2772 Signed-off-by: phatlc <phatle.hsd@gmail.com>
hsdfat
force-pushed
the
fix-pom-close-deadlock-2772
branch
from
September 21, 2026 06:41
cab9c38 to
2736904
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With
Consumer.Offsets.AutoCommit.Enable = false,PartitionOffsetManager.Close()never returns unless something else callsCommit()orClose()on theOffsetManager. The repro from the issue isManagePartitionfollowed bypom.Close().pom.Close()callsAsyncClose()and then ranges overpom.errors. That channel is only closed bypom.release(), which runs fromreleaseSelectedPOMs. Without auto-commit,mainLoopisn't started, so only an explicitom.Commit()orom.Close()reaches it. TheOffsetManager.Closedoc says to close the POMs first, and doing that deadlocks. The existing manual-commit tests close theOffsetManagerbefore the POM to work around it.Now, when auto-commit is disabled,
pom.Close()releases the POM from its parent itself. Two details:Closedrainspom.errors. A concurrentCommit()can be blocked sending an error to a fullpom.errors(withConsumer.Return.Errors) while holdingpomsLockfor reading. Taking the write lock before draining would deadlock, and draining first unblocks it.Closestill waits for the release to finish before it returns.releasePOMonly releases the POM if it is still the one managing its partition. So callingCloseagain on a POM that was already released can't release a newer POM for the same partition, which releasing by topic/partition would do.Behaviour change: in manual-commit mode,
pom.Close()releases the POM without committing its marked offset, so callers need toCommit()first. This is whatremovePartitionsdoes for revoked partitions and whatOffsetManager.Close()does for the remaining POMs in this mode. Before, the only wayClosereturned was when another goroutine'sCommit()orClose()released the POM. I added one sentence about this to theClosedoc on the interface. Auto-commit mode is unchanged.I considered and rejected:
Closesend a commit the user didn't ask for, and neither of the other manual-mode release paths does that.AsyncClose. It is called withpomsLockread-held fromasyncClosePOMsandremovePartitions, so taking the write lock there would deadlock. It would also drop an offset that a laterCommit()is meant to flush.The consumer group session never calls
pom.Close(); it usesremovePartitionsandom.Close().release()is guarded by async.Once, and the POM is removed from the map under the write lock, so a laterCommit/Closedoesn't see it again.pom.lockisn't held whenpomsLockis taken, which keeps the existingpomsLock→pom.lockorder.TestPartitionOffsetManagerCloseWithoutAutoCommithas three cases. Each fails with a 5s timeout on main:ChannelBufferSize = 1andReturn.Errors, one failingCommit()fillspom.errorsand a second one blocks sending its error. The test waits until that commit holdspomsLock, then checks thatClosereturns both errors. Releasing before draining deadlocks here.AsyncCloses the new POM, and closes the old one a second time. The new POM is still managed.go test -race .passes, apart fromTestClientBackgroundMetadataUpdaterandTestPartitionConsumerComputeBackoff, which fail the same way on main when the whole package runs (a leftover goroutine logging through the swappedLogger).golangci-lint runreports 0 issues.Fixes #2772