On Sat, 31 Jan 2026 17:13:27 GMT, Artur Barashev <[email protected]> wrote:

> > > 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.
> 
> I can't find anything like `The intent of intern() in X509Factory (as 
> documented) is to return a canonical cached instance when possible.` in 
> `intern` method's javadoc. Is there some other documentation? If we assume 
> that there is indeed such requirement, then we should replace soft cache with 
> a hard cache implementation.

 I see what you’re saying. What I’m relying on is a best-effort interpretation 
of the javadoc wording:

“`If the given X509Certificate or X509CertImpl is already present in the cert 
cache, the cached object is returned. Otherwise it is added to the cache and 
returned`.”

My intent here is to preserve that behavior at the time of the call: return the 
cached instance if one is present, otherwise insert and return one while still 
keeping the cache soft and narrowing the lock scope compared to the old 
synchronized methods.

Without synchronization in addIfNotPresent, that intent can be weakened under 
concurrency, as concurrent callers may observe different instances even while 
an entry effectively exists in the cache. This transiently diverges from the 
“return cached if present” behavior described above.

If the consensus is that even best-effort convergence under concurrency isn’t 
important here, we could discuss simplifying further, though that would 
slightly relax the current behavior. Happy to change based on where we want to 
land on this.

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

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

Reply via email to