nickfujita: Fix: Message buffering, connection handling, and concurrent connection support#873
nickfujita: Fix: Message buffering, connection handling, and concurrent connection support#873
Conversation
…ple tcp packages'
…ies' into tdrz/nickfujita-improvements
|
🚀 Deployed on https://697d09c3e82b834a966e558b--pglite.netlify.app |
|
I've not had a chance yet to look in detail (I'm tied up this morning in a mob session) - but I did ask GPT5.2 for a review of the branch. It looks like it's spotted a few things. PR 873 Review —
|
samwillis
left a comment
samwillis
left a comment
There was a problem hiding this comment.
@tdrz sorry for the delay! looking really good - I was all set to approve but Opus found a missing await, thats the main this. There are a few other things it's found too:
PR Review: Connection Multiplexer for PGLite Socket Server
Summary
This PR implements a connection multiplexer allowing multiple clients to share a single PGlite instance via a QueryQueueManager that serializes query execution while maintaining transaction isolation per connection. The approach is sound and the implementation handles the core use cases well.
What's Working Well ✅
-
Transaction tracking is correctly integrated - The socket server uses
execProtocolRaw, and I verified thatisInTransaction()works correctly because transaction state is tracked at the WASM write callback level (#pglite_write), which parsesCommandCompleteMessagefor all protocol execution paths. -
Handler ID tracking - Each connection gets a unique ID for transaction attribution, enabling correct isolation.
-
Transaction-aware queue processing - When a transaction is active, only queries from the transaction owner are processed, preventing interleaving.
-
Cleanup on disconnect -
clearQueueForHandlerrejects pending queries andclearTransactionIfNeededrolls back orphaned transactions. -
Good integration test coverage - Tests cover:
- Interleaved transaction and query from different clients
- Transaction owner disconnect/crash scenarios
- Two independent interleaved transactions
Issues Found
1. 🔴 Critical: Missing await on ROLLBACK
async clearTransactionIfNeeded(handlerId: number): Promise<void> {
if (this.db.isInTransaction() && this.lastHandlerId === handlerId) {
this.db.exec('ROLLBACK') // ← Missing await!
this.lastHandlerId = null
await this.processQueue()
}
}The rollback may not complete before processQueue() is called, potentially causing the next query to execute while the rollback is still in progress. This could lead to undefined behavior or transaction state corruption.
Fix:
await this.db.exec('ROLLBACK')2. 🟡 Medium: Potential Indefinite Blocking
When a transaction is active but the transaction owner has no queries in the queue, processQueue() breaks out of the loop:
if (i === -1) {
query = null
}
if (!query) breakIf the transaction owner is slow (e.g., user thinking, network delay), other clients' queries will sit in the queue indefinitely until the owner sends another query or disconnects.
Scenario:
- Client A sends
BEGIN→ transaction starts - Client B sends
SELECT 1→ queued, waiting - Client A is idle for 30 seconds...
- Client B's query is blocked the entire time
Consider adding a warning log when queries are blocked, or a configurable timeout for blocked queries.
Test Coverage Gaps
The integration tests are good, but there are some gaps:
-
No unit tests for
QueryQueueManager- The handler tests mock it, so the actual transaction queue logic isn't unit tested in isolation. -
No test for slow/idle transaction owner - Would be valuable to verify behavior when the transaction owner doesn't send queries for an extended period.
-
No explicit queue ordering verification - No test explicitly verifies that transaction owner's queries are prioritized correctly when multiple handlers have queued queries.
Minor Observations
-
The
CONNECTION_QUEUE_TIMEOUTconstant (line 5) appears to be exported but unused after the refactor to the new multiplexing approach. -
Consider adding JSDoc documentation to
QueryQueueManagerexplaining the transaction isolation strategy.
Verdict
The architecture is solid and the transaction isolation approach is correct. Please fix the missing await on the ROLLBACK before merging. The blocking concern is worth noting but acceptable for an initial implementation - it could be addressed in a follow-up if it becomes an issue in practice.
|
@samwillis Thank, good points! Fixed the await. The rest are all valid points, will address them in the future. |
3 participants
See #775