Auto-reconnect on BrokenPipe outside transactions#510
Open
ntninja wants to merge 8 commits intodenodrivers:mainfrom
Open
Auto-reconnect on BrokenPipe outside transactions#510ntninja wants to merge 8 commits intodenodrivers:mainfrom
BrokenPipe outside transactions#510ntninja wants to merge 8 commits intodenodrivers:mainfrom
Conversation
Author
|
This can be manually tested:
This applies to bewCloud, for instance. |
There was a problem hiding this comment.
Pull request overview
This PR adds handling for Deno.errors.BrokenPipe to improve resiliency when a PostgreSQL connection is dropped, aiming to auto-reconnect for non-transaction queries and at transaction start, while surfacing TransactionError for in-transaction failures so applications can restart transactions (Fixes #436).
Changes:
- Retry a non-transaction query once after
BrokenPipeby closing and reconnecting. - Attempt to handle
BrokenPipeduringTransaction.begin()by reconnecting and retryingBEGIN. - Broaden
TransactionErrorto carry aBrokenPipecause; adjust error handling paths and connection shutdown.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
client.ts |
Adds BrokenPipe reconnect-and-retry logic around query execution. |
query/transaction.ts |
Adds BrokenPipe handling in transaction lifecycle and wraps BrokenPipe as TransactionError. |
client/error.ts |
Expands TransactionError cause type to include BrokenPipe. |
connection/connection.ts |
Makes end() more robust by wrapping termination write in try/finally and waiting on writer readiness. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
@ntninja Hey, thanks for the contribution, please make the relevant updates to ensure checks pass, and also review copilot suggestions. |
ntninja
force-pushed
the
main
branch
2 times, most recently
from
742dd5e to
544c69b
Compare
ntninja
force-pushed
the
main
branch
2 times, most recently
from
67eda8c to
89fbd77
Compare
…ng transaction command Automatically reconnecting for an in-transaction queries is not supported, as we’d have to replay all previous queries and still risk inconsistency if any subsequently issued application queries depended on data previously read during the disconnected transaction. We’d *also* have to keep a log of all previously returned data to verify that it hasn’t changed when reissuing the command – an unacceptable overhead, particularly since applications must be able to handle transaction conflicts anyway and this patch just makes disconnects look like any other “please retry transaction” condition. This commit also: * Removes useless `try…catch` blocks around function calls already containing an equivalent `try…catch` block and hence never catching anything. * Replaces calls to `commit()` in error handling with `rollback()` as `COMMIT`ing after an putting a transaction into an “aborted” state actually issues a `ROLLBACK` internally in PostgreSQL (probably some arcane compatiblity thing with software like this one), but using the wrong terminology is highly confusing. (*Actually* committing would risk applying inconsistent changes, resulting in data corruption!)
…stal Time) in tests
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.
Reconnects to PostgreSQL when receiving
BrokenPipeupon executing freestandig query or transaction start.In-transaction queries are not retried, since we’d have to replay all previous queries and still risk inconsistency if a subsequently issued queries depended on data previously read during the aborted transaction. (We’d also have to keep a log of all previously returned data to verify that it hasn’t changed when reissuing the command!)
This patchset does convert
BrokenPipetoTransactionErrorto let applications know that they should restart the transaction: If the database is really down, the subsequent.begin()will then fail with aConnectionError.This allow removes the calls to
.commit()in the transaction error handling, see last commit message for why and why this doesn’t actually change application observable behaviour.Fixes #436