From aea44b9d8f223e201b5a4a2f0d85206880e76de5 Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 5 Dec 2016 13:48:22 +0100 Subject: [PATCH] SONAR-8332 fix broken thread safety of UUID generation --- .../sonar/core/util/UuidGeneratorImpl.java | 18 +++--- .../core/util/UuidGeneratorImplTest.java | 61 ++++++++++++++++++- 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/sonar-core/src/main/java/org/sonar/core/util/UuidGeneratorImpl.java b/sonar-core/src/main/java/org/sonar/core/util/UuidGeneratorImpl.java index f76adf7e0cc..2b3b13d7f92 100644 --- a/sonar-core/src/main/java/org/sonar/core/util/UuidGeneratorImpl.java +++ b/sonar-core/src/main/java/org/sonar/core/util/UuidGeneratorImpl.java @@ -38,7 +38,6 @@ public final class UuidGeneratorImpl implements UuidGenerator { } private static class UuidGeneratorBase { - private final byte[] buffer = new byte[15]; // We only use bottom 3 bytes for the sequence number. Paranoia: init with random int so that if JVM/OS/machine goes down, clock slips // backwards, and JVM comes back up, we are less likely to be on the same sequenceNumber at the same time: private final AtomicInteger sequenceNumber = new AtomicInteger(new SecureRandom().nextInt()); @@ -46,7 +45,7 @@ public final class UuidGeneratorImpl implements UuidGenerator { // Used to ensure clock moves forward private long lastTimestamp = 0L; - void initBase(int sequenceId) { + void initBase(byte[] buffer, int sequenceId) { long timestamp = System.currentTimeMillis(); synchronized (this) { @@ -70,7 +69,7 @@ public final class UuidGeneratorImpl implements UuidGenerator { System.arraycopy(secureMungedAddress, 0, buffer, 6, secureMungedAddress.length); } - protected byte[] generate(int increment) { + protected byte[] generate(byte[] buffer, int increment) { // Sequence number adds 3 bytes putLong(buffer, increment, 12, 3); @@ -93,21 +92,26 @@ public final class UuidGeneratorImpl implements UuidGenerator { @Override public byte[] get() { + byte[] buffer = new byte[15]; int sequenceId = getSequenceId(); - initBase(sequenceId); - return super.generate(sequenceId); + initBase(buffer, sequenceId); + return super.generate(buffer, sequenceId); } } private static class FixedBasedUuidGenerator extends UuidGeneratorBase implements WithFixedBase { + private final byte[] base = new byte[15]; + FixedBasedUuidGenerator() { int sequenceId = getSequenceId(); - initBase(sequenceId); + initBase(base, sequenceId); } @Override public byte[] generate(int increment) { - return super.generate(increment); + byte[] buffer = new byte[15]; + System.arraycopy(base, 0, buffer, 0, buffer.length); + return super.generate(buffer, increment); } } } diff --git a/sonar-core/src/test/java/org/sonar/core/util/UuidGeneratorImplTest.java b/sonar-core/src/test/java/org/sonar/core/util/UuidGeneratorImplTest.java index 21200b0aa40..175aec720c6 100644 --- a/sonar-core/src/test/java/org/sonar/core/util/UuidGeneratorImplTest.java +++ b/sonar-core/src/test/java/org/sonar/core/util/UuidGeneratorImplTest.java @@ -19,10 +19,13 @@ */ package org.sonar.core.util; +import java.util.ArrayList; import java.util.Base64; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -31,7 +34,7 @@ public class UuidGeneratorImplTest { private UuidGeneratorImpl underTest = new UuidGeneratorImpl(); @Test - public void generate_returns_unique_values_without_common_initial_letter_given_more_than_one_milisecond_between_generate_calls() throws InterruptedException { + public void generate_returns_unique_values_without_common_initial_letter_given_more_than_one_millisecond_between_generate_calls() throws InterruptedException { Base64.Encoder encoder = Base64.getEncoder(); int count = 30; Set uuids = new HashSet<>(count); @@ -49,6 +52,33 @@ public class UuidGeneratorImplTest { } } + @Test + public void generate_concurrent_test() throws InterruptedException { + int rounds = 500; + List uuids1 = new ArrayList<>(rounds); + List uuids2 = new ArrayList<>(rounds); + Thread t1 = new Thread(() -> { + for (int i = 0; i < rounds; i++) { + uuids1.add(underTest.generate()); + } + }); + Thread t2 = new Thread(() -> { + for (int i = 0; i < rounds; i++) { + uuids2.add(underTest.generate()); + } + }); + t1.start(); + t2.start(); + t1.join(); + t2.join(); + + Base64.Encoder encoder = Base64.getEncoder(); + Set uuids = new HashSet<>(rounds * 2); + uuids1.forEach(bytes -> uuids.add(encoder.encodeToString(bytes))); + uuids2.forEach(bytes -> uuids.add(encoder.encodeToString(bytes))); + assertThat(uuids).hasSize(rounds * 2); + } + @Test public void generate_from_FixedBase_returns_unique_values_where_only_last_4_later_letter_change() { Base64.Encoder encoder = Base64.getEncoder(); @@ -68,4 +98,33 @@ public class UuidGeneratorImplTest { assertThat(iterator.next()).startsWith(base); } } + + @Test + public void generate_from_FixedBase_concurrent_test() throws InterruptedException { + UuidGenerator.WithFixedBase withFixedBase = underTest.withFixedBase(); + int rounds = 500; + List uuids1 = new ArrayList<>(rounds); + List uuids2 = new ArrayList<>(rounds); + AtomicInteger cnt = new AtomicInteger(); + Thread t1 = new Thread(() -> { + for (int i = 0; i < rounds; i++) { + uuids1.add(withFixedBase.generate(cnt.getAndIncrement())); + } + }); + Thread t2 = new Thread(() -> { + for (int i = 0; i < rounds; i++) { + uuids2.add(withFixedBase.generate(cnt.getAndIncrement())); + } + }); + t1.start(); + t2.start(); + t1.join(); + t2.join(); + + Base64.Encoder encoder = Base64.getEncoder(); + Set uuids = new HashSet<>(rounds * 2); + uuids1.forEach(bytes -> uuids.add(encoder.encodeToString(bytes))); + uuids2.forEach(bytes -> uuids.add(encoder.encodeToString(bytes))); + assertThat(uuids).hasSize(rounds * 2); + } } -- 2.39.5