Potential data race in ByteBufferPool

[gregw]

Jetty version
9.4.x

Description

A data race detector was run over jetty 9.4.34 and flagged a potential race at
ByteBufferPool.java:199 in org.eclipse.jetty.io.ByteBufferPool$Bucket.release(Ljava/nio/ByteBuffer;)V

[joakime]

Line 199 points to a System.nanoTime line in 9.4.34 ?

https://github.com/eclipse/jetty.project/blob/jetty-9.4.34.v20201102/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java#L197-L201

https://github.com/eclipse/jetty.project/blob/e46af88704a893fc12cb0e3bf46e2c7b48a009e7/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java#L197-L201

[eamonnmcmanus]

Right, there's no synchronization around the assignment to _lastUpdate or its access in getLastUpdate(), and _lastUpdate is not volatile, so this is technically not thread-safe. I think the worst that can happen is that the wrong buffer gets chosen for recycling, so this is more about performance than correctness.

[lorban]

Yeah, some JVM implementations might modify the long in two 32-bit steps, so a concurrent thread could read a corrupt timestamp. As pointed by @eamonnmcmanus that timestamp is only used to figure out which bucket to clean up so this could have some perf side-effects when that race condition occurs but that should be about it.

Nevertheless, that field should be marked volatile.

[lorban]

I used AtomicLong to pay a slightly lower cost than the volatile field accesses by using lazySet() / get() on JDK 8 and setOpaque() / getOpaque() on JDK 11.