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

fix(transaction): fixed unhandled rejection when connection acquiring times out (#9218) #9879

Merged
merged 4 commits into from Sep 7, 2018

Conversation

@AlexKorn
Copy link
Contributor

@AlexKorn AlexKorn commented Sep 5, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Error described in issue #9218 happens when connection acquiring for auto-callback transaction times out: sequelize tries to automatically rollback transaction, but there is no connection to send rollback command.
Added check for inexistent connection, test and special erorr type.

@@ -273,6 +274,7 @@ class ConnectionManager {
return promise.then(() => {
return this.pool.acquire(options.priority, options.type, options.useMaster)
.then(mayBeConnection => this._determineConnection(mayBeConnection))
.catch(/TimeoutError/, err => { throw new errors.ConnectionAcquireTimeoutError(err); })

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 6, 2018
Contributor

Throw this error in _determineConnection, we also we want to cover cases of replication

This comment has been minimized.

@AlexKorn

AlexKorn Sep 6, 2018
Author Contributor

Maybe I miss your point, but when TimeoutError happens, process doesn't fall in _determineConnection? Or the point is that check should be copied to _determineConnection?

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 6, 2018
Contributor

In case of timeout error mayBeConnection should be TimeError instance, which can be used to throw ConnectionAcquireTimeoutError inside _determineConnection

This is important because _determineConnection is used by both common pool and replication pool. Otherwise you will need to apply this same check to three different places (where pool.acquire is used)

This comment has been minimized.

@AlexKorn

AlexKorn Sep 6, 2018
Author Contributor

Timeout error is unfortunately exception from case described in #8330 - it is really thrown by pool, so _determineConnection is not called. By the way, is replication pool used directly, bypassing getConnection() method of connection-manager? Search didn't show me such cases, isn't call to _determineConnection redundant in replication pool acquire() method?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 6, 2018

Dedicated error for acquire timeout has been talked about before and I think your approach is good.

Could you also work toward an integration test which demonstrate issue with transaction. Limit pool to 1 connection and try to run two transactions. That should be sufficient as a test case. Please also test if similar error which we are trying to resolve in this PR appears by that test without your fix.

@codecov
Copy link

@codecov codecov bot commented Sep 6, 2018

Codecov Report

Merging #9879 into master will increase coverage by 5.03%.
The diff coverage is 100%.

@AlexKorn AlexKorn force-pushed the AlexKorn:bugfix/9218 branch from 7468e32 to 8719b5a Sep 6, 2018
@AlexKorn
Copy link
Contributor Author

@AlexKorn AlexKorn commented Sep 6, 2018

fixed pool integration test concerning this issue: not in pretty way but I'm stuck trying to imagine another. The problem is that this unhandled error is so tricky that it bypasses promise catches, so .eventually.rejectedWith doesn't catch it either. Now if test doesn't pass, it just crashes node process

@AlexKorn
Copy link
Contributor Author

@AlexKorn AlexKorn commented Sep 6, 2018

The worst about this error that error message is misleading: first line of call stack says that it happens in promise at this line (for postgres):

new Promise((resolve, reject) => this.client.query(this.sql, (error, result) => error ? reject(error) : resolve(result)));

but in fact it's not the main culprit. It's the line

this.sequelize.log('Executing (' + (this.client.uuid || 'default') + '): ' + this.sql, this.options);

where uuid param of inexistent this.client is referenced, causing process to fail with synchronous error before promise is returned, that's why all .catch()-es doesn't work

@@ -273,6 +274,7 @@ class ConnectionManager {
return promise.then(() => {
return this.pool.acquire(options.priority, options.type, options.useMaster)
.then(mayBeConnection => this._determineConnection(mayBeConnection))
.catch(err => err.name === 'TimeoutError', err => { throw new errors.ConnectionAcquireTimeoutError(err); })

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 7, 2018
Contributor

Short version catch('TimeoutError', err => { ... })

Add this check to

return this.pool.read.acquire(priority)
and
return this.pool.write.acquire(priority)

This comment has been minimized.

@AlexKorn

AlexKorn Sep 7, 2018
Author Contributor

Don't know whether you've seen my question: it seems that acquire() method of replication pool proxy object is never called directly except from getConnection() method, where all necessary checks are made, so wouldn't be catch() here and moreover check for mayBeConnection redundant?

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 7, 2018
Contributor

You are right, for both replication and general case, this.pool.acquire in getConnection represent all calls. Please go ahead and remove mayBeConnection check from replication pool proxy's acquire case.

Apart from that just simplify catch('TimeoutError', err => { ... }) and this should be good to go

This comment has been minimized.

@AlexKorn

AlexKorn Sep 7, 2018
Author Contributor

Removed checks.
Considering catch() simplification: string didn't work. It could be a regex, but then it tries to match with whole stack, therefore fails test for errors in mssql/connection-manager. Found in bluebird docs the way to make it a bit shorter with object predicate.

.to.eventually.be.rejectedWith('ResourceRequest timed out');
return expect(this.testInstance.transaction(() => {
return this.testInstance.transaction(() => {});
})).to.eventually.be.rejectedWith(Sequelize.ConnectionAcquireTimeoutError);

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 7, 2018
Contributor

All ok

@@ -273,6 +272,7 @@ class ConnectionManager {
return promise.then(() => {
return this.pool.acquire(options.priority, options.type, options.useMaster)
.then(mayBeConnection => this._determineConnection(mayBeConnection))
.catch({name: 'TimeoutError'}, err => { throw new errors.ConnectionAcquireTimeoutError(err); })

This comment has been minimized.

@sushantdhiman

sushantdhiman Sep 7, 2018
Contributor

We could have used https://github.com/coopernurse/node-pool/blob/master/lib/errors.js#L18 but looks like it is not exported. Not super critical either

This comment has been minimized.

@AlexKorn

AlexKorn Sep 7, 2018
Author Contributor

Yeah, I also wondered whether it was exposed, but not

@sushantdhiman sushantdhiman merged commit 27e4494 into sequelize:master Sep 7, 2018
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 91.01%)
Details
codecov/project 96.04% (+5.03%) compared to 342e18f
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 7, 2018

Thanks @AlexKorn

@papandreou
Copy link

@papandreou papandreou commented Sep 7, 2018

Do you think this would be backportable to v4?

papandreou added a commit to peakon/sequelize that referenced this pull request Sep 7, 2018
Cherry-pick of 27e4494
@papandreou papandreou mentioned this pull request Sep 7, 2018
2 of 5 tasks complete
@papandreou
Copy link

@papandreou papandreou commented Sep 7, 2018

Do you think this would be backportable to v4?

Seems to apply cleanly. Here goes nothing: #9889

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

Successfully merging this pull request may close these issues.

None yet

3 participants