Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Statement timeout in transaction returns a broken connection to the pool #566

Closed
joux3 opened this issue Nov 19, 2018 · 6 comments
Closed

Comments

@joux3
Copy link

joux3 commented Nov 19, 2018

Expected behavior

After a statement timeout in transaction, the used connection will be working properly when used again from the pool.

Actual behavior

pg-promise does not rollback the transaction when a statement timeout has happened in it. This connection will be in a transaction aborted state and next user of it from the pool can not run any queries.

Steps to reproduce

  1. run a transaction
  2. cause a statement timeout in that transaction
  3. try to reuse to connection and see that it fails

Here is a repro case: https://gist.github.com/joux3/a29727968f7195eb640959013fab319f

Environment

  • Version of pg-promise: 8.5.2
  • OS type (Linux/Windows/Mac): Mac
  • Version of Node.js: 10.12.0

Extra info

The code field for the statement timeout error is 57014. This causes the rollback not the be issued:

function isConnectivityError(err) {
const code = err && typeof err.code === 'string' && err.code;
const cls = code && code.substr(0, 2); // Error Class
return code === 'ECONNRESET' || cls === '08' || cls === '57';
// Code 'ECONNRESET' - Connectivity issue handled by the driver.
// Class 08 - Connection Exception.
// Class 57 - Operator Intervention.
//
// ERROR CODES: https://www.postgresql.org/docs/9.6/static/errcodes-appendix.html
}

@vitaly-t
Copy link
Owner

vitaly-t commented Nov 19, 2018

That code was an attempt to mediate a long-standing issue in the driver. And as such, it is, of course, not perfect, I only provided for the most common cases of losing a connection during a long transaction. If you have a different case, and can offer an improvement over this work-around, a PR is welcome.

Ideally, we all want to see the issue fixed in the driver, but I'm tired of chasing it, without result.

@vitaly-t
Copy link
Owner

vitaly-t commented Nov 19, 2018

P.S. There is a chance that this PR resolved the original issue, but it needs a thorough testing.

@joux3
Copy link
Author

joux3 commented Nov 22, 2018

That does seem annoying, thanks for your hard work shielding us users from dependency issues. I presume pg-promise could just issue rollbacks on any transaction error unless the driver was broken in this way.

I opened PR #568 with statement timeout special case handling.

@vitaly-t
Copy link
Owner

@joux3 as per my comments earlier, there is a chance that the whole issue was resolved in the latest driver.

So please try this first:

function isConnectivityError(err) {
    return false;
}

and see if your error still happens or not.

@joux3
Copy link
Author

joux3 commented Nov 27, 2018

Always returning false from isConnectivityError does fix my issue (the PR just causes false to be returned for this exact case). I agree, this looks like the optimal path.

Luckily this issue does not cause me to hurry: I could work around this specific issue in my specific case by using a task instead of a transaction, which of course is not always possible.

Is there something I could do the help test the driver changes? At least modifying the repro case to an actual pg-promise test case should not be too much work.

vitaly-t added a commit that referenced this issue Nov 30, 2018
@vitaly-t
Copy link
Owner

vitaly-t commented Nov 30, 2018

@joux3 After applying all the latest updates, some tests and considerations, I ended up implementing the very same change as was originally in your PR.

The reason is, even though the library now can work without function isConnectivityError, it works better/smarter with that verification. So I left it, amended with the query cancellation exception.

It was all released in version 8.5.3.

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

No branches or pull requests

2 participants