Skip to content
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

Fix racing on route handler's indexes change #2605

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

franz1981
Copy link

@franz1981 franz1981 commented May 6, 2024

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

if (failure && !hasNextFailureHandler(context) || !failure && !hasNextContextHandler(context)) {
which still uses a racy check (which doesn't advance the indexes, really) - meaning that match can still later fail.
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?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant