Httpclient connector deadlock

[mnitchev]

Greetings,

We noticed a bug similar to #400 in the reauthentication logic in openstack4j-httpclient connector. This is the code in question in HttpExecutorServiceImpl:

private <R> HttpResponse invokeRequest(HttpCommand<R> command) throws Exception {
    CloseableHttpResponse response = command.execute();

    if (command.getRetries() == 0 && response.getStatusLine().getStatusCode() == 401 && !command.getRequest().getHeaders().containsKey(ClientConstants.HEADER_OS4J_AUTH))
    {
        try
        {
            OSAuthenticator.reAuthenticate();
            command.getRequest().getHeaders().put(ClientConstants.HEADER_X_AUTH_TOKEN, OSClientSession.getCurrent().getTokenId());
            return invokeRequest(command.incrementRetriesAndReturn());
        } finally {
            response.close();
        }
    }

    return HttpResponseImpl.wrap(response);
}

The problem is that the connection created on this line is not being closed before recursively calling the same method. The code in the finally block is executed after the recursive invocation completes which means that a new connection will be requested without closing the currently active one first.

By default Apache's HttpClient only allows 2 concurrent connections for one route. It is possible that two concurrent requests to the same route could fail to authenticate at the same time which will trigger two reauthentications and two recursive invocations of invokeRequest.
That unsuccessfully tries to create two new connections for the same route because the quota for allowed connections would be already filled with the initial connections. That causes a deadlock.

Here's the example workflow that caused our problem:

-> GET route1
<- returns 401 (token has expired)
-> POST route2 (token reauthentication) (1)

-> GET route1
<- return 401 (token has expired)
-> POST route2 (token reauthentication) (2)

(1) <- returns 201
-> GET route 1 (this blocks because there are already two active connections for the same route)

(2) <- return 201
-> GET route 1 (this blocks because there are already two active connections for the same route)

The solution is to move the return method after the finally block. That would guarantee that the initial connection is always closed before the recursive invocation. We're not sure if the other connectors have similar problems, but it's probably a good idea to apply the change to them as well.

[dbantchovski]

In order to execute reauthentication /in case of expired token/ it is necessary a second connection to be allocated and used. The original connection stays open during reauthentication request processing and it will be closed just after reauthentication completes. This means that we need an additional connection to be used during reauthentication and in case of two simultaneous re-authentications this leads to exhausting of the default connection pool – two connections per route. The solution is to close the first connection before to execute reauthentication.

[gdankov]

@auhlig Can you look into the issue?

[mnitchev]

Thanks @auhlig for merging the pull request!

[auhlig]

Thanks for contributing @mnitchev :)

[naddame]

The connection should be closed before OSAuthenticator.reAuthenticate();