Lazily start IncrementingUuidGenerator sessions

[mpkorstanje]

⚡️ What's your motivation?

Even when not used, the IncrementingUuidGenerator is instantiated with each Cucumber execution. This somewhat fundamental to the way the ServiceLoader mechanism works.

Each time an incrementing generator is created, a new session is started for that generator. This means that after 255 executions all sessions are exhausted.

Unfortunately, when using the JUnit Platform, Maven issues a discovery request for each individual class. And Cucumber typically participates in discovery along with JUnit Jupiter. So after requests for 255 classes, Cucumber fails discovery as seen in #2930.

Fixes #2930
Fixes #2925

🤔 What's changed?

Lazily start IncrementingUuidGenerator sessions

🏷️ What kind of change is this?

• 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

@jkronegg best I could think of. Got any other ideas?

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

---

[jkronegg]

TLDR: I suggest adding a default UuidGenerator.initialize() method which does nothing, but is implemented by IncrementingUuidGenerator, and add some test-cases. I can update the PR if you want.

---

The issue #2925 can reproduced easily in UuidGeneratorServiceLoaderTest with two different patterns:

• many calls to the loadUuidGenerator() method on one UuidGeneratorServiceLoader instance
• calling once loadUuidGenerator() on many UuidGeneratorServiceLoader instance

This corresponds to the two following unit tests:

@Test
void test_case_2_no_exception_when_calling_loadUuidGenerator_many_times() {
    Options options = () -> null;
    UuidGeneratorServiceLoader loader = new UuidGeneratorServiceLoader(
            UuidGeneratorServiceLoaderTest.class::getClassLoader,
            options);
    UuidGenerator actual = null;
    for (int i=0; i<1000; i++) {
        actual = loader.loadUuidGenerator();
    }
    assertThat(actual, instanceOf(RandomUuidGenerator.class));
}

@Test
void test_case_2_no_exception_when_instantiating_UuidGeneratorServiceLoader_many_times() {
    Options options = () -> null;
    UuidGenerator actual = null;
    for (int i=0; i<1000; i++) {
        UuidGeneratorServiceLoader loader = new UuidGeneratorServiceLoader(
                UuidGeneratorServiceLoaderTest.class::getClassLoader,
                options);
        actual = loader.loadUuidGenerator();
    }
    assertThat(actual, instanceOf(RandomUuidGenerator.class));
}

This somewhat fundamental to the way the ServiceLoader mechanism works.

I think it's more related to the logic used to select the UUID generator. When calling the UuidGeneratorServiceLoader.loadUuidGenerator the generator will be selected based on the logic summarized in the test file:

#	uuid-generator property	Available services	Result
1	undefined	none	exception, no generators available
2	undefined	RandomUuidGenerator, IncrementingUuidGenerator	RandomUuidGenerator used
3	RandomUuidGenerator	RandomUuidGenerator, IncrementingUuidGenerator	RandomUuidGenerator used
4	undefined	RandomUuidGenerator, IncrementingUuidGenerator, OtherGenerator	OtherGenerator used
5	RandomUuidGenerator	RandomUuidGenerator, IncrementingUuidGenerator, OtherGenerator	RandomUuidGenerator used
6	undefined	RandomUuidGenerator, IncrementingUuidGenerator, OtherGenerator, YetAnotherGenerator	exception, cucumber couldn't decide multiple (non default) generators available
7	OtherGenerator	RandomUuidGenerator, IncrementingUuidGenerator, OtherGenerator, YetAnotherGenerator	OtherGenerator used
8	IncrementingUuidGenerator	RandomUuidGenerator, IncrementingUuidGenerator, OtherGenerator, YetAnotherGenerator	IncrementingUuidGenerator used
9	IncrementingUuidGenerator	RandomUuidGenerator, IncrementingUuidGenerator	IncrementingUuidGenerator used
10	OtherGenerator	none	exception, generator OtherGenerator not available
11	undefined	OtherGenerator	OtherGenerator used
12	undefined	IncrementingUuidGenerator, OtherGenerator	OtherGenerator used
13	undefined	IncrementingUuidGenerator	IncrementingUuidGenerator used

In other words, a custom generator written outside Cucumber has the priority over the RandomUuidGenerator and the IncrementingUuidGenerator, even if the cucumber.uuid-generator property is not specified. To implement this behavior, it is mandatory to iterate over the ServiceLoader<UuidGenerator>.iterator(), thus to instantiate the each generator. I don't think we should change this logic (even if the UUID generator is not very popular within the Cucumber community, changing behavior is risky).

Iterating the ServiceLoader more than once could be avoided by using caching, but we would need to take a lot of parameters into account (discard the cache when the cucumber.uuid-generator property changes or when the classloader changes or loads new classes). But this would be very complex and error-prone due to the many different existing usages of Cucumber.

Lazy loading is the only simple solution that I can see (see the Hyrum's law ;-) ), but I think it should be done only once and not on every call to generateId() (because it would decrease the performance). Instead, I suggest introducing a default initialize() method which is called only for the selected generator. Something like:

public interface UuidGenerator extends Supplier<UUID> {
    UUID generateId();

    default UUID get() {
        return generateId();
    }

   default void initialize() {
   }
}

In the UuidGeneratorServiceLoader:

....
UuidGenerator selectedUuidGenerator = ...// decide which generator to use
selectedUuidGenerator.initialize();
...

In the IncrementingUuidGenerator:

public class IncrementingUuidGenerator implements UuidGenerator {
    ...
    public void initialize() {
        initializeMsb();
    }
}

[mpkorstanje]

I think this will do for now.

The ideal solution would be to have the UuidGenerator return a supplier instance that keeps the state internally.

public interface UuidGenerator {

    Supplier<UUID> supplier();
}

And then have the UuidGeneratorServiceLoader return that supplier only for the selected generator. Fortunately the UuidGenerator is experimental so I don't feel too bad about breaking the contract. But I'd rather avoid doing that on a patch version.

[francoissey]

I have just upgrade to 7.20.1 and I run into this issue, could we please reopen it?

[jkronegg]

@francoissey it seems that you are experiencing another bug variant on this mechanism. Could you please open a new issue to describe your setup and behaviour ? (BTW, this thread is a merged Pull Request: it's not supposed to be reopened)

[Theo-Hafsaoui]

Hello, I might be experiencing an issue caused by this regression. Reproducing it is however cumbersome due to the heavy deployment process, and I was wondering if you could estimate which version this bug was introduced in?

[mpkorstanje]

Please start a new issue. Otherwise we got nothing to work off.

And be sure to use the cucumber-bom to enforce dependency alignment as well as mvn dependency:tree -Dverbose to verify alignment. You may be pulling in an older dependency that doesn't have the fix just yet.