Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Do not read from stream with content length of 0 #629

Merged

Conversation

HofmeisterAn
Copy link
Contributor

What does this PR do?

The pull request addresses an issue where Docker.DotNet throws a SocketException when attempting to read from a stream that has an initial content length of 0 bytes.

Why is it important?

The Podman container runtime produces a slightly different HTTP response for the HTTP PUT API /containers/{id}/archive, where it sets the Content-Length field to 0.

Docker

< HTTP/1.1 200 OK
< Api-Version: 1.41
< Date: Wed, 26 Apr 2023 18:04:19 GMT
< Docker-Experimental: false
< Ostype: linux
< Server: Docker/20.10.23 (linux)
< Transfer-Encoding: chunked
<

Podman

< HTTP/1.1 200 OK
< Api-Version: 1.41
< Libpod-Api-Version: 4.5.0
< Server: Libpod/4.5.0 (linux)
< X-Reference-Id: 0x400067c368
< Date: Wed, 26 Apr 2023 18:27:51 GMT
< Content-Length: 0
<

Docker.DotNet does not consider the initial content length and instead tries to read from the stream, resulting in System.Net.Http.HttpRequestException : Error while copying content to a stream. Since the content length is 0, it is acceptable for Podman to terminate the HTTP connection.

  Error Message:
   System.Net.Http.HttpRequestException : Error while copying content to a stream.
---- System.IO.IOException : Unable to read data from the transport connection: Connection reset by peer.
-------- System.Net.Sockets.SocketException : Connection reset by peer
  Stack Trace:
     at System.Net.Http.HttpContent.LoadIntoBufferAsyncCore(Task serializeToStreamTask, MemoryStream tempBuffer)
   at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
   at Docker.DotNet.DockerClient.PrivateMakeRequestAsync(TimeSpan timeout, HttpCompletionOption completionOption, HttpMethod method, String path, IQueryString queryString, IDictionary`2 headers, IRequestContent data, CancellationToken cancellationToken)
   at Docker.DotNet.DockerClient.MakeRequestAsync(IEnumerable`1 errorHandlers, HttpMethod method, String path, IQueryString queryString, IRequestContent body, IDictionary`2 headers, TimeSpan timeout, CancellationToken token)
----- Inner Stack Trace -----
   at Microsoft.Net.Http.Client.ContentLengthReadStream.ReadAsync(Byte[] buffer, Int32 offset, Int32 count, CancellationToken cancellationToken)
   at System.IO.Stream.<CopyToAsync>g__Core|29_0(Stream source, Stream destination, Int32 bufferSize, CancellationToken cancellationToken)
   at System.Net.Http.HttpContent.LoadIntoBufferAsyncCore(Task serializeToStreamTask, MemoryStream tempBuffer)
----- Inner Stack Trace -----

Related issues

-

@HofmeisterAn
Copy link
Contributor Author

@galvesribeiro Could you please merge this fix? It will help us in improving container runtime support beyond Docker.

@galvesribeiro
Copy link
Member

Hey @HofmeisterAn sorry, missed this.

Lets merge this as it should be the best practice regardless of how Docker works anyway.

However, I'd like to express my thoughts on Podman and other containers runtimes. This in particular was a very simple change and as I said it should be that way nonetheless but, I would rather avoid to have other-runtime-specific caveats into Docker.DotNet. The reason is because some things may be not compatible or behave differently making the support difficult.

In other hand, I do see as a good direction as part of the refactory to rethink the support of anything that runs under containerd-based runtimes (i.e. soon WASM containers). We would need to think what is the common API surface (which will clearly exclude things like Swarm which would be left for an "extensions" package) among the runtime implementations and start from it.

Anyway, will digest this and get back to you later on Slack.

Thanks! Will cook a package later Today.

@galvesribeiro galvesribeiro self-requested a review May 15, 2023 17:53
@galvesribeiro galvesribeiro merged commit a0eba61 into dotnet:master May 15, 2023
@HofmeisterAn HofmeisterAn deleted the bugfix/connection-reset-by-peer branch May 15, 2023 18:14
@HofmeisterAn
Copy link
Contributor Author

Thanks 👍.

I would rather avoid to have other-runtime-specific caveats into Docker.DotNet.

Yes, I agree and would also like to avoid it. However, Testcontainers for Node is a good example that there are likely very few, if any, runtime-specific cases required to support the essential features. If we can enhance runtime support through simple fixes, I believe it is something we should do. If it becomes more challenging or specific for a particular runtime, then of course it is out of scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants