f4b6a3 / uuid-creator

UUID Creator is a Java library for generating Universally Unique Identifiers.
MIT License
431 stars 44 forks source link

v1 and v6 are not entirely threadsafe #21

Closed eXsio closed 3 years ago

eXsio commented 4 years ago

Hello. Thanks for this great lib!

I have an issue with thread safety. I'm using v6 (which is slightly modified version of v1 from what I cal tell) and I'm able to generate identical UUIDs from parallel threads.

It seems that the sequence counter is not thread safe at all. My current workaround is to pass the external seq generated by AtomicInteger.

It would be great however if this gets fixed, especially cause the doc is mentioning thread safety.

Cheers!

fabiolimace commented 4 years ago

Hello, @eXsio!

Could you give an example of how to replicate this issue and how you solved it?

I will use this information to fix the problem and to add more test cases.

Best regards

fabiolimace commented 4 years ago

Hi, @eXsio !

I couldn't wait your answer because I try to fix issues as soon as I can in the weekends.

I found out that some tests could fail showing a false alert of duplicate UUIDs. It was caused by an UuidOverrunException thrown when the PC is able to generate more than 10 million UUIDs per second. I confirmed this in a notebook that is faster than my PC.

I just fixed the failing tests. Can you confirm if its your case?

This was the exception:

Exception in thread "Thread-1" com.github.f4b6a3.uuid.exception.UuidCreatorException: The system overran the generator by requesting too many UUIDs.
    at com.github.f4b6a3.uuid.strategy.timestamp.DefaultTimestampStrategy.next(DefaultTimestampStrategy.java:143)
    at com.github.f4b6a3.uuid.strategy.timestamp.DefaultTimestampStrategy.getNextCounter(DefaultTimestampStrategy.java:125)
    at com.github.f4b6a3.uuid.strategy.timestamp.StoppedTimestampStrategy.getTimestamp(StoppedTimestampStrategy.java:59)
    at com.github.f4b6a3.uuid.creator.AbstractTimeBasedUuidCreator.create(AbstractTimeBasedUuidCreator.java:139)
    at com.github.f4b6a3.uuid.creator.AbstractUuidCreatorTest$TestThread.run(AbstractUuidCreatorTest.java:144)

And this was one of the failing tests caused by the exception:

java.lang.AssertionError: A duplicate UUID was created expected:<38976> but was:<29772>
    at org.junit.Assert.fail(Assert.java:88)
    at org.junit.Assert.failNotEquals(Assert.java:834)
    at org.junit.Assert.assertEquals(Assert.java:645)
    at com.github.f4b6a3.uuid.creator.rfc4122.TimeBasedUuidCreatorTest.testGetTimeBasedParallelGeneratorsShouldCreateUniqueUuids(TimeBasedUuidCreatorTest.java:248)

I also included the AtomicInteger for more protection against collisions when the developer uses MULTIPLE generators in the same JVM. It was not necessary because the all generators synchronized. And the class that controls the clock sequence is also sincronized. Even though I decided to use that synchronizer to prevent similar issues in the future.

The average throughput decreased by 17% for time-based generators. This small loss of performance is not bad when it comes to more robustness and less code to maintain.

This issue will remain open awaiting your answer. Thank you!

The version 2.7.7 will be available in the next few hours.

eXsio commented 4 years ago

Hi! Sorry for the silence, I was pretty busy. I didn't register any exceptions other than sql exceptions caused by trying to insert duplicated keys.

I will try to work on a test to replicate the issue.

fabiolimace commented 3 years ago

@eXsio ,

If this problem occurs again, open another issue.

Thanks and greetings!