DefaultJWKSetCache thread safety?

[Former user]

Looking at source code of related to remote jwks, I don't see any volatile/concurrent controll access to it - nor in RemoteJWKSet itself, not it cache.

Maybe I'm missing something, but looks like cache now is missing at least volatile modifier for private JWKSet jwkSet; field.

[Dmitry Pavlov]

Just in case - I created this issue, did not notice that I was not logged in.

[Yavor Vasilev]

Thanks, we’ll examine this.

[Yavor Vasilev]

Yep, the DefaultJWKSetCache is not thread safe as it is. If the cache is shared between threads and an update isn’t seen by some thread, it will trigger its own update. This doesn’t affect correctness, but will cause overhead.

Unfortunately making JWKSet volatile is not enough because there is another flag associated with it - putTimestamp . The volatile has to cover both the JWK set and the timestamp as one variable. I suppose this can be done.

‌

[Vladimir Dzhuvinov]

Commit 1ff5261 : Makes DefaultJWKSetCache thread-safe

Release as v9.3.

[Dmitry Pavlov]

@Vladimir Dzhuvinov thanks for the fix! Just for the sake of completeness - I’m concerned a bit with the fact that content of jwtSetWithTimestamp is not saved to local var in methods where it’s accessed multiple times. Given the fact that in put method it can be set to null, this can lead to NPEs in places that interact with this field multiple times.

I bet the chance is not possible with current code base as I don’t see a call to put with null arg though, so it does not seem like a thing that requires quick fix.

[Vladimir Dzhuvinov]

@Dmitry Pavlov You’re welcome! While we’re at this, would you be interested in doing a thorough look at potential concurrency issues in the lib and devising solutions for them? With perhaps setting up a checker framework, if that makes sense. I can allocate a budget for this work.

[Dmitry Pavlov]

@Vladimir Dzhuvinov I’m not expert enough to do concurrency audit, especially as a payed activity, but NY holidays in Russia are long enough to take a look at a codebase, no doubt :). Would you like extra pair of eyes in general or in some particular pieces of jose lib codebase?

[Vladimir Dzhuvinov]

@Dmitry Pavlov Yay I want to have those long holidays in Russia :)

Concurrency is something that hasn’t properly been looked at in the library, by that I mean in a more comprehensive and systematic way. I don’t like having unknowns and uncertainties in the code and I’d like to have this aspect studied and the code hardened where needed. I want to feel good about the code here.

There are two pathways where people may decide to use the library in a concurrent or async fashion:

1. The creation of JOSE objects - the JWSSigner and JWEEncrypter classes fall into this category
2. The processing / verification of JOSE objects - the JWSVerifier and JWEDecrypter classes, plus those classes which make up the mini framework for checking the signature / decrypting / checking the claims of a received JOSE object

What I’d find very good is to have the concurrency addressed in a systematic, reliable way. I don’t feel well in code bases where aspects of code correctness, like concurrency, are not explicitly captured, be it at language, framework or test level (in that order). You’ve noticed that there is use of the JCIP annotations, but they are purely informational, like a JavaDoc tag. I’ve seen that the Java checker framework has annotations to enforce checking of issues that may affect concurrency, but is that going to work here? I’ve also come across this mini testing framework, which takes an empirical approach by hitting code with multiple threads for some period of time and checking for glitches https://vmlens.com .

What are your thoughts on this topic?

‌

[Vladimir Dzhuvinov]

Commit c2ead98 makes DefaultJWKSetCache.put NPE safe in concurrent scenarios

[Vladimir Dzhuvinov]

Released as version 9.4.1 (2021-01-08).

[Dmitry Pavlov]

@Vladimir Dzhuvinov specific tests are interesting idea, but there are no magic in terms of their applicability as there is still a need to apply them to proper places. I’m going to take a look at places you mentioned this week first.

[Vladimir Dzhuvinov]

Thanks! Java is not the language for concurrency magic, but I believe we can get somewhere close there.