Fix race condition in PekkoRouteHolder using synchronized

[drghdtjr]

In Pekko HTTP’s asynchronous execution model, multiple threads may access the same PekkoRouteHolder instance concurrently.
The class previously relied on a non-thread-safe StringBuilder and mutable fields without synchronization, which allowed concurrent route-matching operations to corrupt internal state.

Changes

Added synchronized to all methods that mutate internal state in PekkoRouteHolder.

Ensured that route path updates and unmatched path state updates occur atomically.

Technical Rationale (Alternatives Considered)

• StringBuffer
  StringBuffer synchronizes individual method calls, but it does not guarantee atomicity across multiple dependent state updates. Since the route path and unmatched path must be updated together to maintain consistency, this approach was insufficient to fully resolve the race condition.

• Immutable String
  Using immutable String objects was also considered. However, frequent string concatenation during Pekko route matching would result in excessive object creation and increased GC pressure, negatively impacting performance.

• synchronized methods (chosen approach)
  Synchronizing state-modifying methods ensures consistency across related fields with minimal structural changes. Given that route matching is not a hot path with heavy contention, the performance impact is negligible while fully addressing the race condition.

issue : #15681

[laurit]

Were you able to reproduce the issue? I’d imagine that while synchronizing might fix the reported exceptions it is doubtful whether it is enough to ensure that the code works correctly.

[drghdtjr]

Hi @laurit,

I have verified the fix with a multi-threaded regression test. Here are the core results:

The Bug Confirmed: Without synchronized, running 100,000 iterations with 50 threads consistently crashed with:

java.lang.ArrayIndexOutOfBoundsException: arraycopy: length -170 is negative

Reason: Concurrent access to ArrayDeque and StringBuilder corrupted internal memory pointers.

The Fix Verified: After applying synchronized, the same stress test passed 100% (100,000/100,000 iterations) without any failures.

Conclusion: This proves that PekkoRouteHolder must be thread-safe to handle Pekko's asynchronous model. All debug logs have been removed, and the test follows project standards.

Please take a look when you have a chance. Thanks!

• without synchronized

• with synchronized

[jaydeluca]

instead of commenting out the code, you could use a @Disabled annotation like here

[drghdtjr]

Hi @jaydeluca,

I commented out the test because PekkoRouteHolder() is private, which causes a compilation error.

Is it the preferred practice in this project to change the visibility to package-private for testing, or should I keep it private and find another way?
Thanks!

[laurit]

This isn't really a meaningful test. It is purely a synthetic tests it is not certain that it actually represents the events that lead to the failure. If this test case represented the actual issue, which based on the test would be that the same PekkoRouteHolder instance is used by multiple requests, then adding the synchronization wouldn't really fix the issue. You could verify it in test by adding a call to holder.push and verifying the result of holder.route() after that. With just the added synchronization some other thread might alter the holder state between push and reading the route.