pgconn: make connection close single-shot and concurrency-safe - #2624
Open
adisetiawanx wants to merge 3 commits into
Open
pgconn: make connection close single-shot and concurrency-safe#2624adisetiawanx wants to merge 3 commits into
adisetiawanx wants to merge 3 commits into
Conversation
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.
Fixes #2622 and, along the way, the same "BUG:" panic family reported in #2332 and #2553. These let a connection torn down from two paths at once panic and kill the whole process instead of failing gracefully.
The problem
PgConn.cleanupDonewas closed from four places:Close,asyncClose, thereceiveMessageOnPgError branch, and CopyFrom's buffering-receive error branch. Each was guarded only by a plain read-then-write ofPgConn.status, a plainbytefield with no synchronization. Two goroutines could both pass the guard and both close the channel, which panics with "close of closed channel". The reproducer in #2622 (a COPY streaming on a raw connection while the transaction context is cancelled) hits this reliably on master.While testing that reproducer against a live PostgreSQL I found the same teardown scenario trips two more defensive panics in the same class:
lock()used a plain store for the busy transition, so two callers could both conclude they owned the connection and the second unlock would have nothing to unlock.The fix
Four changes, split into logical commits:
markClosed()transitions the connection to closed with an atomic swap so exactly one goroutine is the closer, andcleanupDoneis closed through async.Once(finishCleanup). All four close paths now go through these two helpers.enter/exitPotentialWriteReadDeadlocknow hold a small mutex across the enter/write/exit lifecycle, so concurrent flushes queue instead of double-arming the timer and panicking. In normal use the mutex is uncontended since only one flush is in progress at a time.lock()a compare-and-swap and reject locking a connection that is still connecting. Only one caller wins the busy transition, which makes the double-unlock unreachable.abortCopyChanon the buffering-receive error path, so the copy writer goroutine stops writing to the connection while teardown proceeds. That path was the only early-return path missing the close the others already did.The
statusfield is now anatomic.Uint32. This is not cosmetic: it removes the data race on the guard itself, makes the single-closer swap possible, and letsIsClosedand lock/unlock work while close paths run. Every read and write site was converted to Load/Store or CAS.Testing
statusguard aroundcleanupDoneis an unsynchronized test-and-set (reproducer included) #2622: on master it panics with "close of closed channel". With this change it runs to completion, printing "survived", repeatedly and across 32, 48 and 64 workers against PostgreSQL 18.pgconntest suite passes against a live PostgreSQL 18.pgconnsuite passes under-race(Go 1.26, Linux).markClosedreports a single winner under concurrency and thatfinishCleanupnever closes the channel twice.Known limitation
Under
-racethe #2622 reproducer still reports a handful of data races inside thepgproto3wire buffer (Frontend.Send,Frontend.Flush) and onpgConn.txStatus. These only appear because the reproducer drives one connection from two goroutines at once (a CopyFrom on the raw connection obtained viasql.Conn.Raw, racing the rollback database/sql spawns on a cancelled context). They do not panic and the run completes, but they are real unsynchronized accesses, so I documented them in #2623 with the details rather than silently bundling a larger frontend change into this PR.