- Notifications
You must be signed in to change notification settings - Fork556
Fix racing on route handler's indexes change#2605
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
franz1981 wants to merge1 commit intovert-x3:4.xChoose a base branch fromfranz1981:4.x_atomic_handler_choice
base:4.x
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
28ecc98 toacb9aa2Compareacb9aa2 to57ea95dCompareSign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
While looking at#2442 I've read some of the historic changes on this part and it's not clear to me yet what the logic and ownership of indexes changes will look like....
For example, reading#739 and#740 I would expect that concurrent updates of indexes, while leaving the indexes thread-unsafe, can causerworsnop/vertx-beans#12, but sadly the original reproducer repo is no longer there :( (ping to@vincentDAO in case he can findhttps://github.com/vincentDAO/vertxbeans-test-heavy somewhere...).
The point is that if concurrent updates can mess up with handlers handling, what we currently d (i.e. read, modify, read again) looks racy and still prone to bugs, although with a smaller chance to make it happen, unless the original bug wasn't at all related racy accesses to the indexes (which instead is my guess, to verify). In this case, my changes can be replaced with a much better#2442 using weaker updates.
Any thoughts?@tsegismont and@vietj or@slinkydeveloper if he remember this distant in time issue?
FYI, this changes just make it "nearly" impossible to break the existing logic, although a racy access on indexes reset (when we set them to 0) can still create some problems because the reset doesn't happen in a single atomic operation (which means that other threads could still observe a context index == 0 and a failure index != 0 or the opposite too!).
I've claimed "nearly", too, because of
vertx-web/vertx-web/src/main/java/io/vertx/ext/web/impl/RouteState.java
Line 992 in28ecc98
That's why I think is beneficial here to document/explain the expect concurrent behaviour and can be a nice improvement for the next releases (5.x?).