RFR: 8345954: Revisit Class Initializers and Locking in X509TrustManagerImpl [v3]

[Koushik Muthukrishnan Thirupattur]

On Thu, 29 Jan 2026 22:48:23 GMT, Koushik Muthukrishnan Thirupattur <duke at openjdk.org> wrote:

>> Refactor sun.security.provider.X509Factory cache access to avoid coarse-grained locking and reduce contention during certificate/CRL interning and parsing.
>> 
>> As per request in [the PR](https://github.com/openjdk/jdk/pull/22616#issuecomment-2524971845), re-visit "the initialisation and locking in this area, e.g. addToCache is a static synchronized method so very coarse locking."
>
> Koushik Muthukrishnan Thirupattur has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8345954: Removing synchronization on getfromcache

> Thanks for the detailed comment. You’re right that this won’t corrupt the cache and that the objects are semantically equivalent. The issue I see isn’t correctness of the cache, but interning/canonicalization semantics.
> 
> I think it could be possible for two values to be inserted for the same logical key: if two threads concurrently miss on `get()`, they can both construct separate `X509CertImpl/X509CRLImpl` instances and then both call `put()`. The cache remains valid, but different callers may observe different object instances for the same certificate/CRL. That breaks the intent of `intern()` as documented (returning a canonical cached instance) and can introduce avoidable object churn.
> 
> The addIfNotPresent() helper keeps the “check-then-insert and return existing” behavior atomic at this layer, without relying on assumptions about the cache implementation. I would be happy to revisit if we agree interning semantics are not required here. Let me know.



> > > So even if there is a race condition then we'll just insert the same value twice. Would it be possible to insert 2 different values for the same key in this class? It doesn't seem so.
> > 
> > 
> > Thanks for the detailed comment. You’re right that this won’t corrupt the cache and that the objects are semantically equivalent. The issue I see isn’t correctness of the cache, but interning/canonicalization semantics.
> > I think it could be possible for two values to be inserted for the same logical key: if two threads concurrently miss on `get()`, they can both construct separate `X509CertImpl/X509CRLImpl` instances and then both call `put()`. The cache remains valid, but different callers may observe different object instances for the same certificate/CRL. That breaks the intent of `intern()` as documented (returning a canonical cached instance) and can introduce avoidable object churn.
> > The addIfNotPresent() helper keeps the “check-then-insert and return existing” behavior atomic at this layer, without relying on assumptions about the cache implementation. I would be happy to revisit if we agree interning semantics are not required here. Let me know.
> 
> I see your point, but since this is `SoftMemoryCache` there is no guarantee it will always return the same exact object for the same key anyhow. The callers of `intern` method can't rely on that. SoftReferences are automatically cleared by the garbage collector in response to memory demand, then the new object will be re-inserted.

Thanks for the clarification - I agree that using a SoftMemoryCache means we can’t guarantee identity stability across time, since entries may be cleared and rebuilt under memory pressure. 

That said, I think the interning semantics are still meaningful and useful while an entry is present. The intent of intern() in X509Factory (as documented) is to return a canonical cached instance when possible. Without an atomic “return existing or insert” step, concurrent callers can still observe different object instances for the same certificate/CRL even when the cache is populated, which weakens the documented intent of the api and can introduce avoidable allocation.

The soft cache limits how long canonicalization can persist, but it doesn’t remove the value of ensuring convergence among concurrent callers while the entry exists. addIfNotPresent() is meant to preserve that best-effort interning behavior in line with the API intent.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/29181#issuecomment-3826952211