Fix #518, #522

[HofmeisterAn]

@galvesribeiro The first commit disposes the HttpClient. Usually, you would like to keep a single instance of the HttpClient for the entire application. I am not certain if that works with the current implementation. That's why I kept it as a non static field, disposing it makes sense then.

The second one removes the extra properties. I double-checked them against the API docs. pull and outputs where part of e43cdd9 and detected by my script (as I said, I didn't really check the other way around).

Closes #518, closes #522.

[HofmeisterAn]

I'll take a look into the broken tests.

[galvesribeiro]

I wouldn't change the behavior of HttpClient on this release. This is very problematic given the way everything else works.

On the new/refactored code, We would use HttpClientFactory instead, and not manage the lifecycle of it anymore.

[HofmeisterAn]

I wouldn't change the behavior of HttpClient on this release. This is very problematic given the way everything else works.

Disposing the client will not change the behavior, it is just cleaner if someone disposes DockerClient. But if you like I can drop the commit. Regarding the tests:

1. CreateContainerAsync_TimeoutExpires_Fails does not make sense. We cannot control when CreateContainerAsync returns a successful result. The test is not deterministic.
2. GetContainerLogs_* are a bit messy. They share an identical setup. I think we can use a Theory in the future to test all kinds of combinations. Right now, we only test a few of them. I made certain that we await the containerLogsTask at the end of each test.

[HofmeisterAn]

Are any adjustments necessary?

[dgvives]

@HofmeisterAn is there any other way to check if the DockerClient timeout is being considered on each call? I couldn't come out with any alternative

[HofmeisterAn]

@dgvives what do you mean by "any other way"? Looking into the DockerClient implementation, it looks like the (default) timeout is not consistent passed to the HTTP calls. Which probably makes sense (at least for calls that deal with streams).

[dgvives]

@HofmeisterAn the default timeout is used to create a CancellationToken for the http request.

https://github.com/dotnet/Docker.DotNet/blob/master/src/Docker.DotNet/DockerClient.cs#L370-L378

            if (timeout != SInfiniteTimeout)
            {
                using (var timeoutTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
                {
                    timeoutTokenSource.CancelAfter(timeout);
                    return await _client.SendAsync(request, completionOption, timeoutTokenSource.Token)
                        .ConfigureAwait(false);
                }
            }

The test to verify that the request is cancelled after the specified timeout was done using a non-streamed call to DockerClient, in this case CreateContainerAsync. I wonder if there is another way to verify that the default timeout is taken into account in the requests.

[HofmeisterAn]

Ah now I get it, you reffering to:

There is nothing we can do to delay CreateContainerAsync (aka HttpClient.SendAsync) deterministic. We cannot control if it responses successful before the timeout.

Somewhere I had an example that behaves (partially) like an HTTP Docker endpoint (I used it to do some load tests). Maybe we can use this, to enable the test again. I will take a look into it (share it) in the next days. Have some nice holidays.

[HofmeisterAn]

@dgvives I use something like this to test or debug the Docker.DotNet implementation.