@Singleton instances are broken in parallel execution

[loveleif]

👓 What did you see?

@Singleton instances do not work in parallel execution, instead one instance per thread is created.

✅ What did you expect to see?

I expected only one instance of @Singleton bound classes to be created:

cucumber-jvm/cucumber-guice/README.md

Lines 72 to 81 in 6ec14f7

	#### Singleton scope
	Cucumber will create just one instance of a class bound in singleton scope
	that will last for the lifetime of all test scenarios in the test run. You
	should use singleton scope if your classes are stateless. You can also use
	singleton scope when your classes contain state but with caution. You should
	be absolutely sure that a state change in one scenario could not possibly
	influence the success or failure of a subsequent scenario. As an example of
	when you might use a singleton, imagine you have an http client that is
	expensive to create. By holding a reference to the client in a class bound in
	singleton scope, you can reuse the client in multiple scenarios.

📦 Which tool/library version are you using?

cucumber-jvm (core+guice) 7.20.1

🔬 How could we reproduce it?

Test case that reproduce the bug: loveleif@1a52642.

1. Bind a class in Singleton scope.
2. Inject the singleton class in a scenario.
3. Start a test suite with parallel execution.
4. There will be multiple instances created, depending on how many threads are used.

📚 Any additional context?

My use case is query language testing for a database. Starting and stopping the database is time consuming, so we re-use the dbms between scenarios to drastically reduce execution time. A custom backend registers before/after all hooks to the glue that starts/stops the database (since regular before/after all hooks can't use dependency injection, but backends have access to Lookup).

[loveleif]

I'm tempted to attempt to try to fix this, if no one else wants to and you agree that this really is an issue :).

As far as I can see, the heart of the issue is that ObjectFactory is not thread safe by design, it has the start()/stop() methods. For this reason ThreadLocalObjectFactorySupplier is used in parallel execution which creates one ObjectFactory per thread, and therefore also one guice injector per thread.

One possible solution might be to let users specify a custom ObjectFactorySupplier class that is only instantiated once. For guice it would then be possible to keep a single injector instance and use across all ObjectFactory instances created.

[mpkorstanje]

I'm afraid this will be harder then it looks. The current architecture is reaching its limits.

But we can definitely update the documentation to describe that an object is a singleton w.r.t to the injector.

For any other fixes it will depend a bit.

1. This would certainly be a breaking change. So any fix would have to be opt-in.

2. To wit, I haven't looked at Guice for a long time, there is only a single scenario scope per injector. If the injector is shared between threads, we would have to make sure the scenario scope is not. Otherwise different tests would leak state into each other.

3. Any fix would have to be limited to the current extension points (object factory, backend, ect).

So if you think you can fix it within the current framework with an opt-in solution that still isolates the scenario scopes, I'd be happy to consider a pull request.

One possible solution might be to let users specify a custom ObjectFactorySupplier class that is only instantiated once. For guice it would then be possible to keep a single injector instance and use across all ObjectFactory instances created.

I'd like to avoid opening up the internals for extension. For any solution in this direction I think it would be better to take a page from JUnit Jupiters extension model and use a hierarchical context store that extensions can use to store state.

But that is a much bigger project.

I think a more feasible solution would be to consider reusing injectors that have the same configuration. Similar to how Spring reuses application contexts between tests.

This could be achieved with a static Map<Class<?> , Injector> in the object factory.

[loveleif]

Thanks @mpkorstanje, I appreciate the feedback. I can see where you're coming from, even though exposing ObjectFactorySupplier seems innocent enough to me (I'm new here though, so I know that don't mean much).

This could be achieved with a static Map<Class<?> , Injector> in the object factory.

For me personally, I would rather keep my current workaround. Which is a similar concept, I have a custom implementation of GuiceFactory with the injector declared in a static field, something like this:

// ObjectFactory for testing with db conf A
class DbConfAObjectFactory extends MyCustomGuiceFactory {
  private final static Injector DB_CONF_A_INJECTOR = createInjectorWithDbConf(...);

  @Override
  Injector injector() {
    return DB_CONF_A_INJECTOR;
  }
}

// ObjectFactory for testing with db conf B
class DbConfBObjectFactory extends MyCustomGuiceFactory {
  private final static Injector DB_CONF_B_INJECTOR = createInjectorWithDbConf(...);

  @Override
  Injector injector() {
    return DB_CONF_B_INJECTOR;
  }
}

...

Anyway, feels like I'm in too deep for a first contribution. So I will keep my workaround, it works so it's not a big issue (thanks to the generous extensibility already provided 🙏). And by the way, generally I've been very pleased with the library so far, thanks for all the work (I'm moving our gherkin tests from a custom framework we have used before to the standard cucumber-jvm one).

[mpkorstanje]

As a smaller contribution, would you like to update the documentation instead?