Fix racing on route handler's indexes change #2605
Open
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.
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 cause rworsnop/vertx-beans#12, but sadly the original reproducer repo is no longer there :( (ping to @vincentDAO in case he can find https://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 in 28ecc98
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?).