Skip to content

pgconn: make connection close single-shot and concurrency-safe - #2624

Open
adisetiawanx wants to merge 3 commits into
jackc:masterfrom
adisetiawanx:fix/pgconn-cleanup-race
Open

pgconn: make connection close single-shot and concurrency-safe#2624
adisetiawanx wants to merge 3 commits into
jackc:masterfrom
adisetiawanx:fix/pgconn-cleanup-race

Conversation

@adisetiawanx

Copy link
Copy Markdown

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.cleanupDone was closed from four places: Close, asyncClose, the receiveMessage OnPgError branch, and CopyFrom's buffering-receive error branch. Each was guarded only by a plain read-then-write of PgConn.status, a plain byte field 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:

  • "slow write timer already active": the final COPY flush and the Close teardown flush can overlap within the 15ms window and both arm the single shared slow write timer. panic: BUG: slow write timer already active #2332 reports this panic under a heavy CopyFrom workload.
  • "cannot unlock unlocked connection": 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:

  1. Make closing single-shot. markClosed() transitions the connection to closed with an atomic swap so exactly one goroutine is the closer, and cleanupDone is closed through a sync.Once (finishCleanup). All four close paths now go through these two helpers.
  2. Serialize the slow write timer. enter/exitPotentialWriteReadDeadlock now 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.
  3. Make 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.
  4. CopyFrom now closes abortCopyChan on 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 status field is now an atomic.Uint32. This is not cosmetic: it removes the data race on the guard itself, makes the single-closer swap possible, and lets IsClosed and lock/unlock work while close paths run. Every read and write site was converted to Load/Store or CAS.

Testing

Known limitation

Under -race the #2622 reproducer still reports a handful of data races inside the pgproto3 wire buffer (Frontend.Send, Frontend.Flush) and on pgConn.txStatus. These only appear because the reproducer drives one connection from two goroutines at once (a CopyFrom on the raw connection obtained via sql.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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

panic: close of closed channel — the status guard around cleanupDone is an unsynchronized test-and-set (reproducer included)

1 participant