Hi Claes,
Maybe I was to quick with my clicking on Send button... If the Key
simply held strong references to individual String attributes,
LocaleObjectCache.cleanStaleEntries would also have to be modified to
make sure it does not remove valid entries that happen to share equal
Key(s) with cleared entries. So instead of this:
private void cleanStaleEntries() {
CacheEntry<K, V> entry;
while ((entry = (CacheEntry<K, V>)queue.poll()) != null) {
map.remove(entry.getKey());
}
}
The method would have to be like this:
private void cleanStaleEntries() {
CacheEntry<K, V> entry;
while ((entry = (CacheEntry<K, V>)queue.poll()) != null) {
map.remove(entry.getKey(), entry);
}
}
(Notice the use of two-argument Map.remove() method in the modified
variant).
Regards, Peter
P.S. I now understand the hypothetical need to have individual String
attributes wrapped with SoftReference(s) in pre-patched Key. The code
maybe relied on the fact that SoftReference(s) to individual String
attributes were cleared together with CacheEntry(s). When they were
cleared, such Keys suddenly only matched themselves (i.e. no other Key
instance would be equal to them). But if Key's SoftReference(s) were not
cleared before corresponding CacheEntry was cleared, cleanStaleEntries()
running concurrently with get() could remove freshly inserted entries
too. This would not be observed as wrong behavior though. Just
sub-optimal performance.
On 02/07/2018 01:12 PM, Peter Levart wrote:
Hi Claes,
I studied the code briefly and understand why BaseLocale.Key now has
to hold a SoftReference to a BaseLocale object when the same object is
also part of CacheEntry which is also a SoftReference. But I don't see
a reason why pre-patch BaseLocale.Key had to hold SoftReference(s) to
individual String attributes. Couldn't it simply hold strong
references to individual String attributes instead? The
LocaleObjectCache.cleanStaleEntryies() would remove cleared
CacheEntry(s) together with corresponding Key(s) in that case too. So
one SoftReference less, do you agree?
I don't know if it is important for LocaleObjectCache.get() to always
return a canonicalized instance per key so that this always holds:
(cache.get(k1) == cache.get(k2)) == k1.equals(k2)
If it is important, then I noticed a pre-existing race that violates
above invariant:
67 CacheEntry<K, V> newEntry = new CacheEntry<>(key,
newVal, queue);
68
69 entry = map.putIfAbsent(key, newEntry);
70 if (entry == null) {
71 value = newVal;
72 } else {
73 value = entry.get();
74 if (value == null) {
75 map.put(key, newEntry);
76 value = newVal;
77 }
78 }
...which can simply be fixed:
CacheEntry<K, V> newEntry = new CacheEntry<>(key, newVal,
queue);
while (true) {
entry = map.putIfAbsent(key, newEntry);
if (entry == null) {
value = newVal;
break;
} else {
value = entry.get();
if (value == null) {
if (map.replace(key, entry, newEntry)) {
value = newVal;
break;
}
}
}
}
Regards, Peter
On 02/07/2018 11:26 AM, Claes Redestad wrote:
Hi Paul,
On 2018-02-06 20:55, Paul Sandoz wrote:
Quick observation:
261 private BaseLocale getBaseLocale() {
262 return (holder == null) ? holderRef.get() : holder;
263 }
This method can return null if the soft ref has been cleared.
But you don’t check in equals:
270 if (obj instanceof Key && this.hash ==
((Key)obj).hash) {
271 BaseLocale other = ((Key) obj).getBaseLocale();
272 BaseLocale locale = this.getBaseLocale();
273 if
(LocaleUtils.caseIgnoreMatch(other.getLanguage(), locale.getLanguage())
good eye!
It seems this wasn't caught by the existing regression tests since
none of them
recreate Locales in that are likely to have been reclaimed, but still
likely to still
be in the CHM (it's a race of sorts since they'll be removed when the
ReferenceQueue
processing happen).
I added a regression test with the smallest and quickest reproducer I
could come up
with that provokes a NPE if we don't check null along with the fix to
Key#equals:
http://cr.openjdk.java.net/~redestad/8196869/jdk.01/
For the normalize(Key) case we can deduce that a !normalized Key will
always have
a strongly referenced BaseLocale and thus not need to deal with
getBaseLocale()
returning null. I clarified this in the code and added an assert
(that would be triggered
by the added test if it wasn't true).
Thanks!
/Claes
