fix(query): send Sync after an error in a rows-limited query#3708
Open
GiHoon1123 wants to merge 1 commit into
Open
fix(query): send Sync after an error in a rows-limited query#3708GiHoon1123 wants to merge 1 commit into
GiHoon1123 wants to merge 1 commit into
Conversation
Error during a rows-limited query permanently wedges the connection. Query#handleError() never sent Sync for queries using the `rows` option (paged Execute+Flush fetching). PostgreSQL waits for Sync before sending ReadyForQuery, so once an error occurred mid-fetch, the connection was left with readyForQuery permanently false and every subsequent query on that client queued forever with no error and no timeout. Route the existing rows-only sync call sites through a shared _syncRows() helper and call it from handleError() too. A _hasSentSync flag prevents a duplicate Sync in the one path that already sent its own (the bind-throw path in prepare()), which already called connection.sync() directly before invoking handleError(). Fixes: brianc#3707
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 #3707.
Problem
Running a
rows-limited query (client.query({ text, rows: N }), which paginates results via repeated Execute+Flush) permanently wedges the connection if a Postgres error occurs partway through fetching. The erroring query itself rejects correctly, but every subsequent query on that same client hangs forever with no error and no timeout.Root cause
rows-limited queries use the extended query protocol's Execute+Flush cycle to fetch pages of results. If Postgres reports anErrorResponsemid-fetch, it then waits for aSyncmessage before it will sendReadyForQuery.Query#handleError()never sentSyncfor therowscase (onlyhandleCommandComplete()/handleEmptyQuery()did, on the success path) — so after an error, the connection was left withreadyForQuerypermanentlyfalse, and the next query queued internally forever.Fix
if (this.rows) { connection.sync() }call sites (handleCommandComplete,handleEmptyQuery) into a shared_syncRows(connection)helper._syncRows(connection)fromhandleError()too, so the error path now sendsSyncjust like the success path already did._hasSentSyncflag so_syncRowsis a no-op if aSyncwas already sent for this query. This matters for one existing path:prepare()'s bind-throw handler already sent its ownconnection.sync()before callinghandleError()— without the flag, that would now send a duplicateSync.Testing
test/integration/gh-issues/3707-tests.js): runs arows: 2query that hits a real division-by-zero error partway through (generate_serieswith a10 / (5 - i)expression), then races a follow-upselect 1against a 4s timer. Verified this test fails with'follow-up hung'(not a CI-hanging timeout - it fails fast within the 4s window) against the code before this fix, and passes after.test/unit/client/throw-in-bind-tests.js): assertsSyncis sent exactly once (not twice) when a bind throws for arows-limited query, covering the_hasSentSyncde-duplication path specifically.test-unitsuite (271 assertions) and thetest-integrationsuite locally against a real Postgres instance - both clean aside from two pre-existing failures in my local environment unrelated to this change (missing SSL cert setup for2085-tests.js, missingpsqlCLI for130-tests.js).