On Tue, 26 Sep 2023 14:46:46 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> A different fix after https://github.com/openjdk/jdk/pull/14506 was closed. >> >> Still haven't made the attributes set immutable but at least it is populated >> before an entry is added to `entries` and it will never be modified later. >> >> I tried the newly added `AttributesMultiThread.java` test hundreds of times >> and only observed failures before this fix (~%2 failure rate). > > src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1270: > >> 1268: } >> 1269: Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH)); >> 1270: return Collections.unmodifiableSet(new >> HashSet<>(entry.attributes)); > > I do not think this fix is correct. A call to `engineGetAttribute()` could > observe an empty `HashSet` set by `populateAttributes`, but before the entry > is populated. If we want to prevent that I believe we need a lock of some > kind shared between `engineGetAttribute` and `populateAttributes`. > Also the constructor of `HashSet` that takes a collection calls > `HashSet.addAll()`, which loops over the provided collection. In a multi > threaded context, if `populateAttributes` can run concurrently with > `engineGetAttribute`, it is not impossible that this loop will get a > `ConcurrentModificationException`, like before. Or am I missing something? Entries is a synchronized map, and populateAttributes is only called on freshly created entries before they are added to the map. I don't see how this could fail. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15909#discussion_r1337365700
