ISE: Connection is still allocated

[amihaiemil]

Apparently, using BasicHttpClientConnectionManager (e.g. inside UnixHttpClient) is not ok since it provides a single connection.

This throws the exception:

final Docker docker = new LocalDocker(
    new File("/var/run/docker.sock")
);
docker.ping();
docker.ping();

Becuase the connection is still allocated from the first call. We should either close the connection after each call somehow (not always wanted, of course), or maybe use PoolingClientConnectionManager?

[amihaiemil]

@llorllale Do you know Apache HttpClient? Want to fix this one?

[llorllale]

@amihaiemil yes, I've used this lib before. This is one of those surprises (there will be more).

Question: do we need this method at all?

    /**
     * Ping the Docker Engine.
     * @return True if it responds with 200 OK, false otherwise.
     * @throws IOException If there's network problem.
     */
    boolean ping() throws IOException;

[llorllale]

@amihaiemil anyway, I can work on this

[amihaiemil]

@llorllale Yes, we need that method. That's the first thing a user does when using an API, right? Try to ping it :D

And the issue is with other methods as well, anyway. I would look into that Pooling connection manager, I think that's the way to go.

[amihaiemil]

@0crat in

[0crat]

@0crat in (here)

@amihaiemil Job #44 is now in scope, role is DEV

[0crat]

@0crat in (here)

@amihaiemil Job #44 is already in scope

[0crat]

Bug was reported, see §29: +15 points just awarded to @amihaiemil/z

[0crat]

The job #44 assigned to @llorllale/z, here is why. The budget is 30 minutes, see §4. Please, read §8 and §9. If the task is not clear, read this and this.

[amihaiemil]

@llorllale This would be a reasearch task, I guess (not sure how much you can figure out in 30min). Of course, it's ok to leave a puzzle with your findings.

[llorllale]

@amihaiemil will this work for you?

    @Override
    public final boolean ping() throws IOException {
        final HttpGet get = new HttpGet(this.baseUri.toString() + "/_ping");
        final HttpResponse response = this.client.execute(get);
        get.releaseConnection();  // http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/client/methods/HttpRequestBase.html#releaseConnection()
        return response.getStatusLine().getStatusCode() == HttpStatus.SC_OK;
    }

[amihaiemil]

@llorllale We should also do that, close the connection wherever possible, but it is not enough. The issue is the way the HttpClient is built: Basic connection manager is used, which has a single connection.

We need Pooling connection manager because we won't be able to close any connection right away. For instance, Container.followLogs() will return an InputStream which will have to be consumed by the user directly, we won't be able to close the connection right before returning the InputStream since then they wouldn't be able to read from it anymore.

As you see, a single HttpClient instance is passed around the library, so if we only rely on closing connections manually, the following snippet won't work:

    final Container container = docker.containers.get(...);
    final InputStream logsStream = container.followLogs(); //here we did not close it
    docker.ping();//fail here, connection is still assigned.

[llorllale]

@amihaiemil got it: we need to support concurrency

[llorllale]

@amihaiemil I think you've done the research already.

I'm think this connection pooling should be done both for unix and non-unix/remote endpoints. So we should probably extract that (huge!) block of code that creates the client connection manager in UnixHttpClient into its own class. That class needs to be somehow fed the maxConnections configuration somehow.

[amihaiemil]

@llorllale yes, we could do that, but not yet. The building of the remote HttpClient will be even funnier: you have to register https protocol (same as unix is registered for UnixHttpClient), with SSL socket factory (something like that), in order to use the supplied certificates and encrypt the request properly.

So, for now, let's not extract anything, just make them work separately and refactor later.
Try to use Pooling cm for the UnixHttpClient, see how it works -- I'm also not sure how to use that properly yet :D it shouldn't be hard, though, I think you just pass it the maxConnections number somehow.

[llorllale]

@amihaiemil sorry for requesting confirmation so much

WDYT of this? Notice the new ctor that accepts Supplier<HttpClient>. It is there solely to leave the ctor UnixHttpClient(final File sockerFile) with just the call to the this ctor.

    /**
     * Ctor.
     * @param socketFile Unix socket on disk.
     */
    UnixHttpClient(final File socketFile) {
        this(() -> {
            final PoolingHttpClientConnectionManager pool = new PoolingHttpClientConnectionManager(
                RegistryBuilder
                    .<ConnectionSocketFactory>create()
                    .register(
                        "unix",
                        new ConnectionSocketFactory() {
                            @Override
                            public Socket createSocket(
                                final HttpContext httpContext
                            ) throws IOException {
                                return UnixSocketChannel.open().socket();
                            }

                            @Override
                            public Socket connectSocket(
                                final int connectionTimeout,
                                final Socket socket,
                                final HttpHost host,
                                final InetSocketAddress remoteAddress,
                                final InetSocketAddress localAddress,
                                final HttpContext context
                            ) throws IOException {
                                socket.setSoTimeout(connectionTimeout);
                                socket.getChannel().connect(
                                    new UnixSocketAddress(socketFile)
                                );
                                return socket;
                            }
                        })
                    .build()
            );
            pool.setDefaultMaxPerRoute(0);
            pool.setMaxTotal(0);
            return HttpClientBuilder.create()
                .setConnectionManager(pool)
                .build();
        });
    }

    /**
     * Ctor.
     * @param client The HttpClient.
     */
    UnixHttpClient(Supplier<HttpClient> client) {
      this(client.get());
    }

    /**
     * Ctor.
     * @param client Decorated HttpClient.
     */
    UnixHttpClient(final HttpClient client) {
        this.client = client;
    }

[amihaiemil]

@llorllale

sorry for requesting confirmation so much

Not a problem. Although you can also make fast PRs and we can discuss there :-? same for me.

Yes, looks nice, given that you have to call those setters on the Pooling cm. Are you sure it cannot accept them through its ctor?

[llorllale]

@amihaiemil yes, I'm sure

[amihaiemil]

@llorllale I see, ok

[llorllale]

@amihaiemil do we agree that adding a test for this requires completing that other task about implementing a socket server?

[amihaiemil]

@llorllale yes, I think we may need that. But for now, you can simply add a second docker.ping() to that IT case... then, we also need to setup Travis or Rultor somehow to run the IT cases as well, that's not yet done :D

[0pdd]

@amihaiemil 2 puzzles #51, #52 are still not solved.

[llorllale]

@amihaiemil can we close this?

• This was solved in PR (#44) Changed ConnectionManager for UnixHttpClient #50 which got merged
• Puzzle UnixHttpClientITCase.java:49-51: This should be... #51 for completing one test left due to impediment on UnixHttpClientTestCase.java:50-52: Implement a unix... #41
• Puzzle UnixHttpClient.java:57-58: Connection pooling is... #52 left for making the connection pool size configurable

[amihaiemil]

@llorllale sure, thanks

[0crat]

Order was finished: +30 points just awarded to @llorllale/z

[0crat]

The job #44 is now out of scope

[0pdd]

@amihaiemil the puzzle #51 is still not solved; solved: #52.