Bad code smell: Is Injector as Singleton bad practice?

[Alberto]

Dear List,

I'm trying to debug an issue (https://github.com/serenity-bdd/serenity-gradle-plugin/issues/9) in a 3rd party framework that uses Guice. The problem has to do with the integration of the framework (serenity-bdd) in a gradle plugin, where some instance isn't recreated when needed.

Inspecting the code, I can summarize what I fell is a bad practice. The code has a Singleton more or less like this (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-model/src/main/java/net/thucydides/core/guice/Injectors.java):

public class Injectors {

    private static Injector injectors;

    public static synchronized getInjector() {

        if (injector == null) {

            injector = Guice.createInjector (new MyModule());

        }

         return injector;

    }

}

and then uses Injectors.getInjector() to get instances of classes everywhere through the code. For instance (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/pages/PageUrls.java#L118-L124):

private static String namedUrlFrom(String annotatedBaseUrl) {

    String pageName = annotatedBaseUrl.substring(NAMED_URL_PREFIX.length());

    EnvironmentVariables environmentVariables = Injectors.getInjector().getInstance(EnvironmentVariables.class);

    return EnvironmentSpecificConfiguration.from(environmentVariables)

        .getOptionalProperty(pageName)

        .orElse(environmentVariables.getProperty(pageName));
}

There are also many places throughout the code where constructors uses the Injector to initialize its fields. For example (see in the sources: https://github.com/serenity-bdd/serenity-core/blob/82cd173792d9f7898791cab0fe8d3a775bfb2825/serenity-core/src/main/java/net/serenitybdd/core/annotations/locators/SmartAnnotations.java#L165-L167):

public SmartAnnotations(Field field, MobilePlatform platform) {
    this(field, platform, WebDriverInjectors.getInjector().getInstance(CustomFindByAnnotationProviderService.class));
}

I have no experience using Guice, and no experience with the framework, so I cannot say if it's very wrong or acceptable. But this (anti?)pattern is giving me hard times trying to debug. As it is now, many functions are quite compact, but I guess we are loosing many benefits of Guice, and I'm not sure if this is a legal use at all.

I have began to rewrite the code and prepare a Pull Request, but it's extremely costly and I'm not sure that I will do better than the current code. So the question for the experts: Is this so bad that it would require a complete rewrite of the relevant code? Am I missing something? 

Best regards,

Alberto

[Alberto]

I'm reposting this in github https://github.com/google/guice/issues/1636, as this forums seems to miss some love.

Please, answer there.