RFR: 8232853: AuthenticationFilter.Cache::remove may throw ConcurrentModificationException

[Daniel Fuchs]

Hi Martin,

On 02/11/2019 16:40, Martin Buchholz wrote:
> Hi Julia,
> 
> I think this is the wrong fix.

I disagree :-), but I understand why you could think that:
see below.

> Looking at the Cache class - all the methods are synchronized, so it 
> looks like any failure is not actually due to concurrent execution, but 
> any bug can be reproduced in a single thread. (please add such a repro 
> to the bug report)

Yes - the trick is that for the ConcurrentModificationException
to happen, you have to have something to remove in the cache when
Cacghe::store() is called.
And the only way for that to happen is to have concurrent
responses adding their credentials to the cache at the
"same" time (where "same" means only after all 401 responses
have been received).
For that to happen, then you need concurrent requests to be sent
before any credentials is put in the cache, so that all requests
will call the authenticator, and all responses "compete" to add
their own successful credentials to the cache. In other words:

req#1 is sent, finds no credentials, gets 401, finds no credential,
       calls authenticator, send Authorization.
req#2 is sent, finds no credentials, gets 401, finds no credential,
       calls authenticator, send Authorization.
req#3 is sent, finds no credentials, gets 401, finds no credential,
       calls authenticator, send Authorization.
req#4 is sent, finds no credentials, gets 401, finds no credential,
       calls authenticator, send Authorization.
req#5 is sent, finds no credentials, gets 401, finds no credential,
       calls authenticator, send Authorization.
...

server waits for all requests to reach this point, then sends back 200
responses. At this point, the client will (concurrently) receive all
the 200 responses, and will try to update its cache for each response
received.

resp#1 comes with 200. Client calls Cache::store. There's nothing in
the cache so Cache::remove(3 args) does nothing.

resp#2 comes with 200. Client calls Cache::store. There's something
in the cache - so Cache::remove(3 args) starts iterating. If the
entries match then the previously stored credentials will be
removed, and ConcurrentModificationException may be thrown (or not).
As you well know ConcurrentModificationException is thrown on best
effort. It doesn't always get thrown...

resp#3 comes and the whole store/remove dance starts again.
etc...

This cannot be reproduced with a single thread, because with
sequential requests, if the server sends back a 401, then
the Cache::remove(1 arg) is called when 401 is received, and that
doesn't loop and can't raise any CME. Then when 200 is received
the obsolete credentials have been already removed from
the cache, and therefore the 3 args Cache::remove doesn't find
anything matching to remove - so CME will never be thrown
either.

You really have to have several concurrent requests/response
competing to authenticate and update the cache to see the CME,
even if though cache is properly synchronized and the CME is
thrown because List::remove() is called by the same thread
that iterates.

> It looks like Cache#remove can be fixed simply by using an explicit 
> iterator and using iterator#remove instead of List#remove.

Yes - we could have used an Iterator. I don't like much LinkedList.
We could have used ArrayList and an explicit Iterator instead.
I suggested using  CopyOnWriteArrayList just to avoid using the
Iterator explicitly. Cache are usually few writes and multiple
reads so that looked appropriate.

Would you still advised using plain ArrayList and an Iterator instead?


> Cache looks fishy in other ways, e.g. why does Cache#remove ignore 
> authscheme?

Right, technical debt I'd say: the HttpClient only supports Basic
authentication so there's really only one scheme: "Basic".
That said, maybe we should clean that up and either have the cache
support multiple schemes properly or reap that out from the cache.

best regards,

-- daniel

> 
> (Yes, proper caching infrastructure should be added to the JDK someday, 
> but that's a big project)
> 
> 
> On Fri, Nov 1, 2019 at 9:48 AM Julia Boes <julia.boes at oracle.com 
> <mailto:julia.boes at oracle.com>> wrote:
> 
>     Hi,
> 
>     This fix addresses an issue in AuthenticationFilter.Cache::remove that
>     can lead to a ConcurrentModificationException. The cache is implemented
>     as LinkedList, the method iterates over the list and potentially
>     removes
>     items without using an iterator.
> 
>     To reproduce the bug, multiple requests are sent asynchronously so that
>     successful response headers might arrive concurrently and compete in
>     updating the cache. This increases the chances to trigger the
>     removal of
>     a previously cached credential while storing the credentials of the
>     next
>     response. Under these circumstances, ConcurrentModificationException is
>     thrown with very high frequency (if not always).
> 
>     The proposed fix replaces LinkedList with CopyOnWriteArrayList to
>     preclude the exception.
> 
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8232853
> 
>     Webrev:
>     http://cr.openjdk.java.net/~jboes/webrevs/8232853/webrev.01/index.html
> 
>     The test fails reliably without the fix, and passes with it (jdk_net
>     tests run 50 times).
> 
> 
>     Regards,
> 
>     Julia
> 
>