Improved support for multiple classloaders in IncrementingUuidGenerator (#2853)

Corrected a test-case which fails rarely but sometimes.
Added possibility to give a specific classloader identifier.

Fixes: #2851

Co-authored-by: Julien Kronegg <julien [at] kronegg.ch>

Author: jkronegg

CHANGELOG.md

@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## [Unreleased]
+- [Core] Improved support for multiple classloaders in IncrementingUuidGenerator ([#2853](https://github.com/cucumber/cucumber-jvm/pull/2853) J. Kronegg)
 ### Changed
 - [TestNG] Update dependency org.testng:testng to v7.9.0
 - [Core] Update dependency io.cucumber:tag-expressions to v6.1.0

cucumber-core/src/main/java/io/cucumber/core/eventbus/IncrementingUuidGenerator.java

@@ -44,10 +44,20 @@ public class IncrementingUuidGenerator implements UuidGenerator {
 
     /**
      * Classloader identifier (MSB). The identifier is a pseudo-random number on
-     * 12 bits. Note: there is no need to save the Random because it's static.
+     * 12 bits. Since we use a random value (and cut it to 12 bits), there is a
+     * small probability that two classloaders generate the same 12 bits random
+     * number. This could lead to UUID collisions if the UUID parts (epoch-time,
+     * session counter and counter) are the same. The default `classloaderId`
+     * (random number cut to 12 bits) has a collision rate of less than 1% when
+     * using 10 classloaders (which leads to a much smaller UUID probability
+     * thanks to the other dynamic parts of the UUID like epoch-time and
+     * counters). If you use multiple classloaders and want to ensure a
+     * collision-free UUID generation, you need to provide the `classloaderId`
+     * by yourself. You can do so using the {@link #setClassloaderId(int)}
+     * method. Note: there is no need to save the Random because it's static.
      */
     @SuppressWarnings("java:S2119")
-    static final long CLASSLOADER_ID = new Random().nextInt() & 0x0fff;
+    static long classloaderId = new Random().nextInt() & 0x0fff;
 
     /**
      * Session counter to differentiate instances created within a given
@@ -65,6 +75,24 @@ public class IncrementingUuidGenerator implements UuidGenerator {
      */
     final AtomicLong counter = new AtomicLong(-1);
 
+    /**
+     * Defines a new classloaderId for the class. This only affects instances
+     * created after the call (the instances created before the call keep their
+     * classloaderId). This method should be called to specify a classloaderId
+     * if you are using more than one class loader, and you want to guarantee a
+     * collision-free UUID generation (instead of the default random
+     * classloaderId which produces about 1% collision rate on the
+     * classloaderId, and thus can have UUID collision if the epoch-time,
+     * session counter and counter have the same values).
+     * 
+     * @param classloaderId the new classloaderId (only the least significant 12
+     *                      bits are used)
+     * @see                 IncrementingUuidGenerator#classloaderId
+     */
+    public static void setClassloaderId(int classloaderId) {
+        IncrementingUuidGenerator.classloaderId = classloaderId & 0xfff;
+    }
+
     public IncrementingUuidGenerator() {
         long sessionId = sessionCounter.incrementAndGet();
         if (sessionId == MAX_SESSION_ID) {
@@ -75,7 +103,7 @@ public IncrementingUuidGenerator() {
         }
         long epochTime = System.currentTimeMillis();
         // msb = epochTime | sessionId | version | classloaderId
-        msb = ((epochTime & MAX_EPOCH_TIME) << 24) | (sessionId << 16) | (8 << 12) | CLASSLOADER_ID;
+        msb = ((epochTime & MAX_EPOCH_TIME) << 24) | (sessionId << 16) | (8 << 12) | classloaderId;
     }
 
     /**

cucumber-core/src/test/java/io/cucumber/core/eventbus/IncrementingUuidGeneratorTest.java

@@ -5,22 +5,31 @@
 import org.junit.jupiter.api.Test;
 
 import java.io.IOException;
+import java.lang.reflect.Field;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.net.URISyntaxException;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.UUID;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class IncrementingUuidGeneratorTest {
 
+    public static final String CLASSLOADER_ID_FIELD_NAME = "classloaderId";
+
     /**
      * Example of generated values (same epochTime, same sessionId, same
      * classloaderId, different counter value):
@@ -92,7 +101,7 @@ void same_thread_generates_different_UuidGenerators() {
     void different_classloaders_generators() {
         // Given/When
         List<UUID> uuids = IntStream.rangeClosed(1, 10)
-                .mapToObj(i -> getUuidGeneratorFromOtherClassloader().generateId())
+                .mapToObj(i -> getUuidGeneratorFromOtherClassloader(i).generateId())
                 .collect(Collectors.toList());
 
         // Then
@@ -160,12 +169,131 @@ private static UUID removeClassloaderId(UUID uuid) {
         return new UUID(uuid.getMostSignificantBits() & 0xfffffffffffff000L, uuid.getLeastSignificantBits());
     }
 
-    private static UuidGenerator getUuidGeneratorFromOtherClassloader() {
+    /**
+     * Create a copy of the UUID without the epoch-time part to allow
+     * comparison.
+     */
+    private static UUID removeEpochTime(UUID uuid) {
+        return new UUID(uuid.getMostSignificantBits() & 0x0ffffffL, uuid.getLeastSignificantBits());
+    }
+
+    /**
+     * Check that classloaderId collision rate is lower than a given threshold
+     * when using multiple classloaders. This should not be mistaken with the
+     * UUID collision rate. Note: this test takes about 20 seconds.
+     */
+    @Test
+    void classloaderid_collision_rate_lower_than_two_percents_with_ten_classloaders()
+            throws NoSuchFieldException, IllegalAccessException {
+        double collisionRateWhenUsingTenClassloaders;
+        List<Double> collisionRatesWhenUsingTenClassloaders = new ArrayList<>();
+        do {
+            // When I compute the classloaderId collision rate with multiple
+            // classloaders
+            Set<Long> classloaderIds = new HashSet<>();
+            List<Integer> stats = new ArrayList<>();
+            while (stats.size() < 100) {
+                if (!classloaderIds
+                        .add(getStaticFieldValue(getUuidGeneratorFromOtherClassloader(null),
+                            CLASSLOADER_ID_FIELD_NAME))) {
+                    stats.add(classloaderIds.size() + 1);
+                    classloaderIds.clear();
+                }
+            }
+
+            // Then the classloaderId collision rate for 10 classloaders is less
+            // than 2%
+            collisionRateWhenUsingTenClassloaders = stats.stream()
+                    .filter(x -> x < 10).count() * 100 / (double) stats.size();
+            collisionRatesWhenUsingTenClassloaders.add(collisionRateWhenUsingTenClassloaders);
+        } while (collisionRateWhenUsingTenClassloaders > 2 && collisionRatesWhenUsingTenClassloaders.size() < 10);
+        assertTrue(collisionRateWhenUsingTenClassloaders <= 2,
+            "all retries exceed the expected collision rate : " + collisionRatesWhenUsingTenClassloaders);
+    }
+
+    @Test
+    void same_classloaderId_leads_to_same_uuid_when_ignoring_epoch_time() {
+        // Given the two generator have the same classloaderId
+        UuidGenerator generator1 = getUuidGeneratorFromOtherClassloader(255);
+        UuidGenerator generator2 = getUuidGeneratorFromOtherClassloader(255);
+
+        // When the UUID are generated
+        UUID uuid1 = generator1.generateId();
+        UUID uuid2 = generator2.generateId();
+
+        // Then the UUID are the same
+        assertEquals(removeEpochTime(uuid1), removeEpochTime(uuid2));
+    }
+
+    @Test
+    void different_classloaderId_leads_to_different_uuid_when_ignoring_epoch_time() {
+        // Given the two generator have the different classloaderId
+        UuidGenerator generator1 = getUuidGeneratorFromOtherClassloader(1);
+        UuidGenerator generator2 = getUuidGeneratorFromOtherClassloader(2);
+
+        // When the UUID are generated
+        UUID uuid1 = generator1.generateId();
+        UUID uuid2 = generator2.generateId();
+
+        // Then the UUID are the same
+        assertNotEquals(removeEpochTime(uuid1), removeEpochTime(uuid2));
+    }
+
+    @Test
+    void setClassloaderId_keeps_only_12_bits() throws NoSuchFieldException, IllegalAccessException {
+        // When the classloaderId is defined with a value higher than 0xfff (12
+        // bits)
+        IncrementingUuidGenerator.setClassloaderId(0xfffffABC);
+
+        // Then the classloaderId is truncated to 12 bits
+        assertEquals(0x0ABC, getStaticFieldValue(new IncrementingUuidGenerator(), CLASSLOADER_ID_FIELD_NAME));
+    }
+
+    @Test
+    void setClassloaderId_keeps_values_under_12_bits_unmodified() throws NoSuchFieldException, IllegalAccessException {
+        // When the classloaderId is defined with a value lower than 0xfff (12
+        // bits)
+        IncrementingUuidGenerator.setClassloaderId(0x0123);
+
+        // Then the classloaderId value is left unmodified
+        assertEquals(0x0123, getStaticFieldValue(new IncrementingUuidGenerator(), CLASSLOADER_ID_FIELD_NAME));
+    }
+
+    private Long getStaticFieldValue(UuidGenerator generator, String fieldName)
+            throws NoSuchFieldException, IllegalAccessException {
+        // The Field cannot be cached because the IncrementingUuidGenerator
+        // class is different at each call (because it was loaded by a
+        // different classloader).
+        Field declaredField = generator.getClass().getDeclaredField(fieldName);
+        declaredField.setAccessible(true);
+        return (Long) declaredField.get(null);
+    }
+
+    private static void setClassloaderId(Class<?> generatorClass, int value)
+            throws IllegalAccessException, NoSuchMethodException, InvocationTargetException {
+        // The Method cannot be cached because the IncrementingUuidGenerator
+        // class is different at each call (because it was loaded by a
+        // different classloader).
+        Method method = generatorClass.getDeclaredMethod("setClassloaderId", int.class);
+        method.setAccessible(true);
+        method.invoke(null, value);
+    }
+
+    /**
+     * Create a fresh new IncrementingUuidGenerator from a fresh new
+     * classloader, and return a new instance.
+     * 
+     * @param  classloaderId the classloader unique identifier, or null if the
+     *                       default classloader id generator must be used
+     * @return               a new IncrementingUuidGenerator instance
+     */
+    private static UuidGenerator getUuidGeneratorFromOtherClassloader(Integer classloaderId) {
         try {
-            return (UuidGenerator) (new NonCachingClassLoader()
-                    .findClass(IncrementingUuidGenerator.class.getName())
-                    .getConstructor()
-                    .newInstance());
+            Class<?> aClass = new NonCachingClassLoader().findClass(IncrementingUuidGenerator.class.getName());
+            if (classloaderId != null) {
+                setClassloaderId(aClass, classloaderId);
+            }
+            return (UuidGenerator) aClass.getConstructor().newInstance();
         } catch (Exception e) {
             throw new RuntimeException("could not instantiate " + IncrementingUuidGenerator.class.getSimpleName(), e);
         }