-
Notifications
You must be signed in to change notification settings - Fork 39
Solve race condition when several calls to apoc.trigger.install are made concurrently. #875
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
Solve race condition when several calls to apoc.trigger.install are made concurrently. #875
Conversation
| try { | ||
| node = tx.createNode(SystemLabels.ApocTriggerMeta); | ||
| node.setProperty(SystemPropertyKeys.database.name(), databaseName); | ||
| } catch (ConstraintViolationException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new transaction or the outer one? because if it is the outer one won't that cause issues for the user later on in the query?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I did not consider this. It looks like the tx is created in withUpdatingTransaction() which is called in the beginning of apoc.trigger.install, apoc.trigger.drop, apoc.trigger.dropAll, apoc.trigger.start, apoc.trigger.stop respectively.
The thread does some work before this method, but it is always commited directly after it. Does that sounds OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that should be fine then I think
…ade concurrently. Also update some licenses after new Neo4j release.
d35d998 to
94e028d
Compare
| // This can happen if two threads try to create the same node concurrently, | ||
| // after both having passed the null check. In this case we can ignore the failing tx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps at least one retry? Failing to update lastUpdated can result in that the change to triggers will never propagate (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also make it safer by changing:
if (getLastUpdate() >= lastUpdate) {
updateCache();
}to something like
if (getLastUpdate() >= lastUpdate || lastUpdate < reallyOld) {
updateCache();
}3ec4097 to
2dad37e
Compare
Also update some licenses after new Neo4j release.
Should be cherry-picked to 5.26.