JettyConnectionMetrics: concurrent access without synchronization

[deadok22]

Describe the bug
The 2 lambda expressions here access the maxConnections and connectionSamples fields without synchronization. They are run in Micrometer threads.
These same fields are updated in Jetty threads and the fields are protected by the connectionSamplesLock monitor otherwise.
The lambda expressions must acquire connectionSamplesLock to ensure correctness.

[jonatan-ivanov]

Thanks for the issue!

Those fields are final so they will not be overwritten and the type of synchronization you wrote is not needed. These objects are non immutable though and I think that's the kind of issue you meant.

maxConnections is a TimeWindowMax and it is synchronized.
connectionSamples is a HashMap which is not thread-safe in itself but if you check how it is used, you should see that writing/reading is synchronized except connectionSamples.size() which if you look at the code, is reading a single int variable which is atomic.

So I'm not sure why extra locking would be needed, if you think it is still needed, could you please explain what could go wrong, a test could be also helpful.

[deadok22]

Hey @jonatan-ivanov,

thanks for taking a look!

These objects are non immutable though and I think that's the kind of issue you meant.

Precisely!

writing/reading is synchronized except connectionSamples.size() which if you look at the code, is reading a single int variable which is atomic.

You are correct that int variable reads are atomic: that means the value read will always be a value that was written to the variable, as opposed to some sort of intermediate state.
The intention of the connectionSamples.size() call is to retrieve the size of the collection at the time the call is made. Without synchronization, there is no guarantee the calling thread will observe changes to the size variable made by other threads.

Here's a test program showing effects of failing to synchronize access to an int variable:

import java.time.Duration;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

class Scratch {
    int aConcurrentlyAccessedInt; // adding the `volatile` modifier or otherwise synchronizing access will yield very different results
    public static void main(String[] args) throws InterruptedException {
        final Scratch s = new Scratch();
        final ExecutorService pool = Executors.newFixedThreadPool(2);
        pool.submit(() -> {
            while (!Thread.interrupted()) {
                for (int i = 0; i < 1_000_000_000; i++) {
                    s.aConcurrentlyAccessedInt = i;
                }
            }
            Thread.currentThread().interrupt();
        });
        pool.submit(() -> {
            for (int i = 0; i < 10_000; i++) {
                try {
                    Thread.sleep(42);
                } catch (InterruptedException e) {
                    Thread.currentThread().interrupt();
                    break;
                }
                System.out.println("value seen: " + s.aConcurrentlyAccessedInt);
            }
        });
        Thread.sleep(Duration.ofSeconds(1).toMillis());
        pool.shutdownNow();
        pool.awaitTermination(1, TimeUnit.SECONDS);
    }
}

That aside, we probably shouldn't rely on that particular implementation of size() being used - it can change independently...

[jonatan-ivanov]

Oh yes, sorry, now I totally see what you meant: visibility.
Since we cannot make size volatile (visibility but not atomicity) we should make accessing it syncronized (visibility and atomicity).

Do you want to open a PR (against 1.14.x) to refactor the lambda that calls size into a syncronized method?
This is probably the simplest thing we can do and the cost should be fairly low since Gauges are not polled frequently (once per publish)?

[deadok22]

Please take a look when you get a chance: #6587