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

Make sure transactions never returns connections to the pool twice #491

Merged
merged 2 commits into from
May 22, 2023

Conversation

fbiville
Copy link
Contributor

@fbiville fbiville commented May 17, 2023

Before this fix, users could run into situation when a transaction
would execute their onClosed callback more than once, meaning
the referenced connection could be returned to the idle list of
its server's connection several times (i.e. the connection ref
could be duplicated and borrowed by more than 1 transaction at a
time, which breaks the fundamental invariant of connections).

This could occur when a transaction was committed or
rolled back* and then, users would run a query with the same transaction
again. This is obviously a misuse of the transaction API but the driver
could fail in a more graceful and more understandable manner.

When that subsequent run executed, a transaction ID mismatch error
would then be returned (since the connection was reset upon the
return to pool after commit/rollback, thus forgetting about its former
transaction ID).
The tx ID mismatch error would then lead to onClosed being called
a second time, and the same connection would end up listed a second time
in its server's tracked idle connection list.

One fix could be to make the pool return idempotent.
However, we felt that the better fix is to respect the invariant
according to which a connection is never returned more than once, thus
making the pool return idempotency irrelevant in practice.

*rollback also happens when closing the transaction or closing the
enclosing session and the transaction is the currently active workload
in that session

Fixes #487

@robsdedude robsdedude changed the title Make sure transactions never return connections twice Make sure transactions never returns connections to the pool twice May 17, 2023
Copy link
Member

@robsdedude robsdedude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔌

robsdedude and others added 2 commits May 17, 2023 15:34
Before this fix, users could run into situation when a transaction
would execute their `onClosed` callback more than once, meaning
the referenced connection could be returned to the idle list of
its server's connection several times (i.e. the connection ref
could be duplicated and borrowed by more than 1 transaction at a
time, which breaks the fundamental invariant of connections).

This could occur when a transaction was committed or
rolled back* and then, users would run a query with the same transaction
again. This is obviously a misuse of the transaction API but the driver
could fail in a more graceful and more understandable manner.

When that subsequent run executed, a transaction ID mismatch error
would then be returned (since the connection was reset upon the
return to pool after commit/rollback, thus forgetting about its former
transaction ID).
The tx ID mismatch error would then lead to `onClosed` being called
a second time, and the same connection would end up listed a second time
in its server's tracked idle connection list.

One fix could be to make the pool return idempotent.
However, we felt that the better fix is to respect the invariant
according to which a connection is never returned more than once, thus
making the pool return idempotency irrelevant in practice.

*rollback also happens when closing the transaction or closing the
enclosing session and the transaction is the currently active workload
in that session

Fixes #487

Signed-off-by: Florent Biville <florent.biville@neo4j.com>
@fbiville fbiville merged commit 5074923 into 5.0 May 22, 2023
@fbiville fbiville deleted the issue_487 branch May 22, 2023 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SessionWithContext.BeginTransaction on a new session fails with error "invalid state 3, expected: [0]"
2 participants