Hi Julia,
Your new iterator approach looks good to me. one minor comment as you are using 'var' language construct the variable name 'au' is not a meaning full name .
Thanks,
Vyom
----- Original message -----
From: Julia Boes <julia.b...@oracle.com>
Sent by: "net-dev" <net-dev-boun...@openjdk.java.net>
To: OpenJDK Network Dev list <net-dev@openjdk.java.net>
Cc:
Subject: [EXTERNAL] Re: RFR: 8232853: AuthenticationFilter.Cache::remove may throw ConcurrentModificationException
Date: Tue, Nov 5, 2019 9:09 PM
Thanks for the feedback everyone.
It looks like Cache#remove can be fixed simply by using an explicit iterator and using iterator#remove instead of List#remove.Cache looks fishy in other ways, e.g. why does Cache#remove ignore authscheme?I changed the implementation to use an iterator instead and check the authscheme:
synchronized void remove(String authscheme, URI domain, boolean proxy) {
- for (CacheEntry entry : entries) {
- if (entry.equalsKey(domain, proxy)) {
- entries.remove(entry);
+ var iterator = entries.iterator();
+ while (iterator.hasNext()) {
+ var au = iterator.next();
+ if (!Objects.equals(au.scheme, authscheme)) continue;
+ if (au.equalsKey(domain, proxy)) {
+ iterator.remove();
}
}
}
WRT to the test, I'd suggest this to make it more maintainable:
static final int REQUEST_COUNT = 5;
static final int URI_COUNT = 6;
67 static final CyclicBarrier barrier =
new CyclicBarrier(REQUEST_COUNT * URI_COUNT);
257 void doClient(List<URI> uris) {
assert uris.size() == URI_COUNT;
258 barrier.reset();
...
263 for (int i = 0; i < REQUEST_COUNT; i++) {Done.
Updated webrev: http://cr.openjdk.java.net/~jboes/webrevs/8232853/webrev.02/index.html
I filed a separate issue to revisit the cache implementation: https://bugs.openjdk.java.net/browse/JDK-8233589
Cheers,
Julia
