Skip to content

Fixed Session#reset() to only send a RESET message #456

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

Merged
merged 5 commits into from
Jan 16, 2018

Conversation

lutovich
Copy link
Contributor

Based on #454.

Connection error can happen while transaction is executing. Closing
transaction after such error should not perform rollback because
connection is dead and database has cleaned up all resources.

Previously close operation tried to perform rollback and failed with
same `ServiceUnavailable` exception as the unsuccessful `#run()`. Error
thrown from `#close()` has to be added as a suppressed error to the one
thrown from `#run()`, when used in try-with-resources block. So code
attempted to add error to itself as suppressed. This resulted in an
`IllegalArgumentException`.

This commit fixes the problem by making `#close()` not perform a
rollback after a fatal connection error.
New condition is the same as old one by reads a bit better.
Session can only have one active (not closed) transaction at a time.
Exception is thrown when starting a transaction on a session that
already has an open one.

Situation when transaction is started multiple times on a session with
an open transaction was not handled correctly. First failure overrode
the reference to the last transaction kept in the session. Later
attempts resulted in a different error saying that there already
exists an open connection.

This commit fixes the problem by making session keep current transaction
reference even if new transaction failed to begin. So callers will
receive consistent errors.
@lutovich lutovich requested a review from zhenlineo January 11, 2018 17:58
@lutovich lutovich force-pushed the 1.5-session-reset-fix branch 4 times, most recently from 1dca59d to 6cb060b Compare January 15, 2018 16:38
Instead of sending RESET message and returning connection back to the
pool. Queries & transactions should still be in control of the
connection lifecycle even after reset. Previous behaviour was
problematic because reset might race with query execution and make
it try to use connection that has been returned to the pool.

Also moved all RESET tests to a dedicated `SessionResetIT`. Made
explicit transaction remain open after termination so that user code
still needs to close it to return connection to the pool.
Commit adds couple stub-server tests to assert that commit and
rollback errors are correctly propagated.
@lutovich lutovich force-pushed the 1.5-session-reset-fix branch from 6cb060b to f3ae1d4 Compare January 16, 2018 10:19
@lutovich lutovich merged commit b23a50b into neo4j:1.5 Jan 16, 2018
@lutovich lutovich deleted the 1.5-session-reset-fix branch January 16, 2018 11:09
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.

1 participant