Thread safety issue with Services class / ServiceLoader

[gavanore]

Describe the bug
I am integrating jjwt 0.12.3 into some software, and have discovered what I think to be a possible threading issue in how the Services class interacts with ServiceLoader. I'm using parallel JUnit5 tests and I spuriously (haven't isolated a reliable repro) get NoSuchElementException coming out of java.util.ServiceLoader. The documentation for this class says it's not thread safe, and this is usually the answer given when people query about NoSuchElementException coming out of the service loader, like so:

java.util.NoSuchElementException
	at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1318)
	at java.base/java.util.ServiceLoader$2.next(ServiceLoader.java:1306)
	at java.base/java.util.ServiceLoader$3.next(ServiceLoader.java:1403)
	at io.jsonwebtoken.impl.lang.Services.loadFirst(Services.java:98)
	at io.jsonwebtoken.impl.DefaultJwtBuilder.compact(DefaultJwtBuilder.java:511)

Various ways to solve the issue - a ThreadLocal cache of service loaders, or synchronize loadFirst/loadAll, or wrap the usage of each service loader in a scoped synchronized block, maybe something like

    public static <T> T loadFirst(Class<T> spi) {
        Assert.notNull(spi, "Parameter 'spi' must not be null.");

        ServiceLoader<T> serviceLoader = serviceLoader(spi);
        if (serviceLoader != null) {
            synchronized(serviceLoader) {
                return serviceLoader.iterator().next();
            }
        }

        throw new UnavailableImplementationException(spi);
    }

It looks like you cache the loaded service for potential re-use in the Builders, but that's an instance variable and since you use a new builder for each token, it's effectively re-searched on the classpath. Another way to solve this issue would be to load service implementations (i.e. the Serializer, Deserializer, CompressionAlgorithm, etc.) once where it makes sense to do so, potentially in a static initializer somewhere, which the JVM would guarantee in a single thread and therefore be safe. It should also be logically OK since this is all classpath stuff that should never change after program start. Something like this would also be good since you won't have a situation where a large number of request threads needing token validation are synchronizing on JJWT internals unwittingly.

So if possible, I'd go for a non-synchronized solution.

To Reproduce
Generate tokens in parallel. I'm using jjwt-jackson as my serializer.

Expected behavior
NoSuchElement is not thrown spuriously during normal operations.

Screenshots
Provided text log above

Workaround
Specify the Serializer when using the builder to have the build method skip service lookup.

Update: I have noticed that DefaultJwtParserBuilder has a subsequent build step that probably steps around this issue by passing resolved services from the ParserBuilder to the Parser. You could do the same for the Builder.

[lhazlewood]

@gavanore excellent catch! Thank you so much for creating this issue. The work for the 0.12.0 release ensured that the ServiceLoader lookup and subsequent cache was thread-safe, but not iteration of the ServiceLoader instance itself. We'll use this issue to track the work for making that thread-safe as well. cc @bdemers

[gavanore]

Glad to be of use. Thank you for creating this library.

[vojkny]

We just stumbled into the same issue, will appreciate the fix as well!

[bdemers]

We are looking into a fix, in the short term you can set the instance of the service, this will cause the ServiceLoader to NOT be used automatically

[lhazlewood]

As @bdemers said, at least until we have a longer term solution for this, you can disable dynamic lookup by explicitly setting the respective service instance as JwtBuilder and JwtParserBuilder properties per https://github.com/jwtk/jjwt#custom-json-processor

When they are explicitly provided, the builders don't need to perform dynamic lookup using ServiceLoader.

[icecreamhead]

Hi, are there any updates on this issue? We're also encountering the problem.

[lhazlewood]

@icecreamhead not just yet, but we've started other work for a follow-up 0.12.4 release, and we can add this to that release. In the meantime, the solution in my #873 (comment) works.

[lhazlewood]

Just to be clear, that looks like this (using Jackson for example):

Jwts.builder().json(new JacksonSerializer())...

or:

Jwts.parser().json(new JacksonDeserializer())...