Skip to content

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

Merged
galvesribeiro merged 1 commit into
dotnet:masterfrom
HofmeisterAn:bugfix/connection-reset-by-peer
May 15, 2023
Merged

fix: Do not read from stream with content length of 0#629
galvesribeiro merged 1 commit into
dotnet:masterfrom
HofmeisterAn:bugfix/connection-reset-by-peer

Conversation

@HofmeisterAn

@HofmeisterAn HofmeisterAn commented Apr 28, 2023

Copy link
Copy Markdown
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 HofmeisterAn mentioned this pull request Apr 28, 2023
Open
10 tasks
@HofmeisterAn

HofmeisterAn commented May 15, 2023

Copy link
Copy Markdown
Contributor Author

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

@galvesribeiro

galvesribeiro commented May 15, 2023

Copy link
Copy Markdown
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

HofmeisterAn commented May 15, 2023

Copy link
Copy Markdown
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

Reviewers

@galvesribeiro galvesribeiro galvesribeiro approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@HofmeisterAn @galvesribeiro