feat: use Local File Detector by default

[JonasPammer]

Resolves #128 (comment) and takes away my need for a custom utility function and reliance on container. as per my current predicament outlined in #134

https://www.selenium.dev/documentation/webdriver/drivers/remote_webdriver/#uploads

[jdaugherty]

@matrei can you please review this one?

[JonasPammer]

TODO: implement ServiceLoader Pattern as done in #143 to allow static overwriteability

[matrei]

This is excellent! Now users don’t have to do anything special for uploads to work seamlessly in ContainerGebSpec.

Looking forward to your addition of a ServiceLoader implementation.

[JonasPammer]

probably heading to do this and others on weekend

Looking forward to your addition of a ServiceLoader implementation.

Since its my first time using this pattern, do you think it'd be ok if we create just one Loader Class for everything (which i cant implement here, as multiple PRs) or one Loader for each corresponding trait/interface (the latter of which i am going to write here)
no-op not-important question, just curious

in the github code search projects i couldnt find much and if, it was always seperate small price utility loader class for one thing. whole idea of this question is i dont want to bloat the code with dozens of loaders in future

[matrei]

Good point! Now that we have created our own Loader with override feature in #143, we could make that more generic (for the class) and specific for the methods. Then we can add loading of different types in that class as separate PR:s after #143 is merged.

An alternative would be to create multiple loaders grouped in a package like grails.geb.serviceloader.*.
I think I actually like this alternative better. Then we get these classes out of the main package and can still have sensible and aligned method names (setInstance()).

[JonasPammer]

may have went overboard, please correct me if simpler better or mistakes

[matrei]

How about we create base service registry:

package grails.plugin.geb

import groovy.transform.CompileStatic

import java.util.concurrent.ConcurrentHashMap

/**
 * A service registry that loads and caches different service types using the Java
 * {@link java.util.ServiceLoader}, while allowing overriding which instance to return.
 * <p>
 * This class provides thread-safe service loading and caching, supporting parallel test execution.
 * It provides both automatic service discovery through {@link java.util.ServiceLoader}
 * and explicit replacement of the returned service instance for customization.
 * </p>
 * <p>
 * Usage example:
 * <pre>
 * MyService service = ServiceRegistry.getInstance(MyService, DefaultMyService)
 * </pre>
 * </p>
 *
 * @since 4.2
 * @author Mattias Reichel
 */
@CompileStatic
class ServiceRegistry {

    private static final ThreadLocal<ConcurrentHashMap<Class<?>, Object>> INSTANCES = ThreadLocal.withInitial {
        new ConcurrentHashMap<Class<?>, Object>()
    }

    /**
     * Returns the service instance of the given service type, loading it using
     * {@link java.util.ServiceLoader} if not already loaded or an instance
     * of the default implementation type if no service implementation is found
     * by the {@link java.util.ServiceLoader}.
     *
     * If an instance has been set using {@link #setInstance(Class, Object)} or
     * {@link #setInstance(Class, Class)}, that instance will be returned instead.
     *
     * @param serviceType The service type
     * @param defaultType The service implementation type to use if no service
     * implementation is found (Must have a zero-argument constructor)
     * @return An instance of the service type
     */
    static <T> T getInstance(Class<T> serviceType, Class<? extends T> defaultType) {
        (T) INSTANCES.get().computeIfAbsent(serviceType) {
            ServiceLoader.load(serviceType)
                    .findFirst()
                    .orElseGet { defaultType.getDeclaredConstructor().newInstance() }
        }
    }

    /**
     * Sets the instance to be returned for the given service type, bypassing instance
     * loading from {@link java.util.ServiceLoader}.
     * <p>
     * Setting the instance to {@code null} will revert to loading the the service instance
     * via the {@link java.util.ServiceLoader}.
     * </p>
     * @param serviceType The service type for which the instance should be set
     * @param instance The instance to return for the given service type, or
     * {@code null} for default service loading
     */
    static <T> void setInstance(Class<T> serviceType, T instance) {
        INSTANCES.get().put(serviceType, instance)
    }

    /**
     * Sets the implementation type to return for the given service type, bypassing instance loading
     * from {@link java.util.ServiceLoader}.
     *
     * @param serviceType The service type for which the instance should be set
     * @param instanceType The type of the instance to return for the given service type.
     * (Must have a zero-argument constructor).
     */
    static <T> void setInstance(Class<T> serviceType, Class<? extends T> instanceType) {
        setInstance(serviceType, instanceType.getDeclaredConstructor().newInstance())
    }
}

then we can delegate to it and do not have to manage any implementation details in the different concrete implementations:

@CompileStatic
class ContainerFileDetectorServiceLoader {

    /**
     * Delegates to {@link ServiceRegistry#getInstance(Class, Class)}.
     */
    static ContainerFileDetector getInstance() {
        ServiceRegistry.getInstance(ContainerFileDetector, DefaultContainerFileDetector)
    }

    /**
     * Delegates to {@link ServiceRegistry#setInstance(Class, Object)}.
     */
    static void setInstance(ContainerFileDetector instance) {
        ServiceRegistry.setInstance(ContainerFileDetector, instance)
    }

    /**
     * Delegates to {@link ServiceRegistry#setInstance(Class, Class)}.
     */
    static void setInstance(Class<? extends ContainerFileDetector> clazz) {
        ServiceRegistry.setInstance(ContainerFileDetector, clazz)
    }
}

or perhaps even cleaner, call the ServiceRegistry directly not needing any different service loader implementations at all:

if (currentConfiguration.fileDetector != NullContainerFileDetector) {
    ServiceRegistry.setInstance(ContainerFileDetector, currentConfiguration.fileDetector)
}
ContainerFileDetector fileDetector = ServiceRegistry.getInstance(ContainerFileDetector, DefaultContainerFileDetector)

What do you think?

[JonasPammer]

I knew about threadlocal, to be honest only through a random claude generation once, but yes with this concurrenthashmap it looks like solid code. Can you tell me why we need this threadlocal or concurrent thing though? If not ill study myself. But yes, totally can do with this

[matrei]

As the Loader class is used statically (same instance for all threads), when running tests in parallel (different threads, in the same JVM process), multiple threads can set different service implementations at the same time, you can't know that the instance that is meant to be used in a test is actually used, it could have been set by another thread. With ThreadLocal each thread gets its own version of the service registry map. Hmm, maybe it does not have to be a ConcurrentHashMap, I had that as a thread safety guard first, but then realized that would not suffice as different threads could still replace each others service instances.

[jdaugherty]

As the Loader class is used statically (same instance for all threads), when running tests in parallel (different threads, in the same JVM process), multiple threads can set different service implementations at the same time, you can't know that the instance that is meant to be used in a test is actually used, it could have been set by another thread. With ThreadLocal each thread gets its own version of the service registry map. Hmm, maybe it does not have to be a ConcurrentHashMap, I had that as a thread safety guard first, but then realized that would not suffice as different threads could still replace each others service instances.

Usually Context type classes use a thread local to ensure isolation. I don't think we need to optimize to share these across threads - the overhead isn't that extreme. Let's change to a ThreadLocal and replace the concurrenthashmap with hashmap?

[matrei]

Actually, TheadLocal might be overkill as well: https://github.com/grails/geb?tab=readme-ov-file#parallel-execution.

[jdaugherty]

Actually, TheadLocal might be overkill as well: https://github.com/grails/geb?tab=readme-ov-file#parallel-execution.

Long term, I was hoping to make it thread safe. I did this as a stop gap for now. I'd rather we go ahead and add threadlocal for future proofing.

[JonasPammer]

Actually, TheadLocal might be overkill as well: https://github.com/grails/geb?tab=readme-ov-file#parallel-execution.

Long term, I was hoping to make it thread safe. I did this as a stop gap for now. I'd rather we go ahead and add threadlocal for future proofing.

Totally, hell yeah

[matrei]

Yeah, but I'm not sure this is going to work as I envisioned. If Spock is run in parallell mode, setupSpec() will probably not run in the same thread as the feature methods, so anything we set there will not be set in the feature methods.

Also, currently I don't think overriding in setupSpec() works:

void setupSpec() {
    ContainerFileDetectorServiceLoader.setInstance(UselessContainerFileDetector)
}

as the condition here will always be true if you don't use the annotation (so this will always run and set it to DefaultContainerFileDetector):

if (currentConfiguration.fileDetector != NullContainerFileDetector) {
    ContainerFileDetectorServiceLoader.setInstance(currentConfiguration.fileDetector)
}

and also WebdriverContainerHolder.reinitialize() seems to run before setupSpec() so the instance will already be set on the driver when setupSpec() runs:

ContainerFileDetector fileDetector = ContainerFileDetectorServiceLoader.getInstance()
((RemoteWebDriver) driver).setFileDetector(fileDetector)

[JonasPammer]

put to draft until i get to doing wished implementation of comments

[JonasPammer]

WebdriverContainerHolder.reinitialize() seems to run before setupSpec() so the instance will already be set on the driver when setupSpec() runs:

ContainerFileDetector fileDetector = ContainerFileDetectorServiceLoader.getInstance()
((RemoteWebDriver) driver).setFileDetector(fileDetector)

Yes, some added println with flushes confirm:

matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.UselessContainerFileDetector)
2025-03-04 07:38:27.867  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : ContainerFileDetectorDefaultSpec does not match grails.plugin.geb.UselessContainerFileDetector@5b2a23e9
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector)


2025-03-04 07:38:36.297  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : ContainerFileDetectorSpockSpec matches grails.plugin.geb.DefaultContainerFileDetector@7c68b975
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector)
ContainerFileDetectorSpockSpec::setupSpec


2025-03-04 07:38:36.730  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : DownloadSupportSpec matches grails.plugin.geb.DefaultContainerFileDetector@328c9bf5
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector)
2025-03-04 07:38:37.528  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : PageDelegateSpec matches grails.plugin.geb.DefaultContainerFileDetector@328c9bf5
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, true, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector)
2025-03-04 07:38:37.751  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : RootPageSpec does not match grails.plugin.geb.DefaultContainerFileDetector@328c9bf5
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, testing.example.com, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, true, class grails.plugin.geb.DefaultContainerFileDetector)
2025-03-04 07:38:45.389  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : ServerNameControllerSpec does not match grails.plugin.geb.DefaultContainerFileDetector@382b008d
matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, testing.example.com, false, class grails.plugin.geb.DefaultContainerFileDetector)
2025-03-04 07:38:52.992  INFO 137172 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : UploadSpec does not match grails.plugin.geb.DefaultContainerFileDetector@437eeb4

matchesCurrentContainerConfiguration grails.plugin.geb.WebDriverContainerHolder$WebDriverContainerConfiguration(http, host.testcontainers.internal, false, class grails.plugin.geb.DefaultContainerFileDetector) == currentConfiguration
2025-03-04 07:37:24.922  INFO 134180 --- [    Test worker] g.plugin.geb.WebDriverContainerHolder    : ContainerFileDetectorSpockSpec matches grails.plugin.geb.DefaultContainerFileDetector@2c5ff4a5
ContainerFileDetectorSpockSpec::setupSpec

Until figure how to circumvent, will stay draft