On Sat, 31 Jan 2026 00:59:22 GMT, Koushik Muthukrishnan Thirupattur <[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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/29181#issuecomment-3828855729
