Skip to content

Don’t swallow the cause of a TX termination. (#731) #737

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

Conversation

gjmwoods
Copy link
Contributor

  • Don’t swallow the cause of a TX termination.

There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen.

The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not.

This PR changes the following:

  • Add a StateHolder to the UnmangedTransaction
  • The holder is necessary to keep the single field volatie
  • The holder holds the state and a possible cause of termination
  • The holder is able to determine whether a session is still open or not.
  • It removes markTerminated from InternalAsyncTransaction as it was used only for tests.

* Don’t swallow the cause of a TX termination.

There are a couple of scenarions in which a transaction gets terminated. The cause will be propagated by the pull handler and the transaction will be marked accordingly. There might be a chance that a degration from a leader to a follower on the server side happens in between to calls to a run method on that transaction: The transaction is still open, but cannot run queries any longer. Outside a transactional function this leads correctly to a ClientException. Inside a transactional function, this must not happen.

The retry logic must be able to find the cause of the termination and if there’s any, it should judge on the cause if it retries or not.

This PR changes the following:

- Add a StateHolder to the UnmangedTransaction
- The holder is necessary to keep the single field volatie
- The holder holds the state and a possible cause of termination
- The holder is able to determine whether a session is still open or not.
- It removes markTerminated from InternalAsyncTransaction as it was used only for tests.

* Polishing.

This removes the circular dependency between holder and state. I had introduced it cause I wanted to avoid object allocation. This solution here is better: It also creates only 3 different holders for non terminated states, but without the circular dependency.

Co-authored-by: Gregory Woods <[email protected]>
Copy link
Contributor

@michael-simons michael-simons left a comment

Choose a reason for hiding this comment

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

LGTM ;) 👍

@gjmwoods gjmwoods merged commit 233e0e4 into neo4j:4.1 Jun 10, 2020
@gjmwoods gjmwoods deleted the 4.1-issue/cause-of-termination-swallowed branch June 10, 2020 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants