vertx-web icon indicating copy to clipboard operation
vertx-web copied to clipboard

Fix racing on route handler's indexes change

Open franz1981 opened this issue 3 months ago • 0 comments

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 https://github.com/rworsnop/vertx-beans/issues/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 https://github.com/vert-x3/vertx-web/blob/28ecc981f02c1840b9cb39bc0e0d87de1ba787fd/vertx-web/src/main/java/io/vertx/ext/web/impl/RouteState.java#L992 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?).

franz1981 avatar May 06 '24 07:05 franz1981