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 for concurrent child transactions failing (#2213) #3440

Merged
merged 3 commits into from Sep 20, 2019

Conversation

@codeclown
Copy link
Contributor

codeclown commented Sep 13, 2019

Fix for #2213 where @joepie91 already did the bulk of the work and investigation. I needed this fixed for a personal project so adapted their test and reimplemented the check for _previousSibling.

@codeclown

This comment has been minimized.

Copy link
Contributor Author

codeclown commented Sep 13, 2019

The new test failed on mssql with the message:

SAVE TRANSACTION trx422 - Can't acquire connection for the request. There is another request in progress.

Probably related to this: tediousjs/node-mssql#302

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 13, 2019

@codeclown I assume situation is not any worse than it was before for mssql in this scenario? If that's the case, probably it's ok to just skip mssql for the test.

@codeclown

This comment has been minimized.

Copy link
Contributor Author

codeclown commented Sep 13, 2019

@kibertoad I'm personally fine with that, it could be noted in the docs where there's a note about Redshift lack of savepoint support as well. I'll update the PR.

I'm not 100% content with the purely timing-based test here, though. I'll see if I could make it more concrete by adding an assurance that the promises resolve in the expected order always.

@codeclown

This comment has been minimized.

Copy link
Contributor Author

codeclown commented Sep 20, 2019

@kibertoad Updated PR, please see 2 commits.

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Sep 20, 2019

Much appreciated!

@kibertoad kibertoad merged commit 5417cac into knex:master Sep 20, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on trx at 87.887%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.