-
-
Notifications
You must be signed in to change notification settings - Fork 9
fix: Handle 101 Upgrade responses by ignoring Content-Length #51
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
base: main
Are you sure you want to change the base?
fix: Handle 101 Upgrade responses by ignoring Content-Length #51
Conversation
lanwen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks legit, please let me know how can I try that. We have a stable reproducer here: https://github.com/ivan-curkovic/tcc-bug-reproduction
Probably the best approach is to use this branch locally or in GH Codespaces. Just change the endpoint of the container runtime: diff --git a/test/Docker.DotNet.Tests/TestFixture.cs b/test/Docker.DotNet.Tests/TestFixture.cs
index b32ecd6..ada3469 100644
--- a/test/Docker.DotNet.Tests/TestFixture.cs
+++ b/test/Docker.DotNet.Tests/TestFixture.cs
@@ -21,7 +21,7 @@ public sealed class TestFixture : Progress<JSONMessage>, IAsyncLifetime, IDispos
public TestFixture(IMessageSink messageSink)
{
_messageSink = messageSink;
- DockerClientConfiguration = new DockerClientConfiguration();
+ DockerClientConfiguration = new DockerClientConfiguration(endpoint: new Uri("http://localhost:12345"));
DockerClient = DockerClientConfiguration.CreateClient(logger: this);
Cts = new CancellationTokenSource(TimeSpan.FromMinutes(5));
Cts.Token.Register(() => throw new TimeoutException("Docker.DotNet tests timed out."));
and run the tests: If You can also just run the tests related to creating and starting containers (that should be enough): Edit: Unfortunately, I don't think we can use the reproducer. TC for .NET isn't compatible with Docker Engine v29 yet, though Docker.DotNet has already been updated. Normally, we could just bump the transitive dependency. |
|
@HofmeisterAn I mean if I can try it as a dependency for the reproducer - is that possible from the local branch? |
That's what my edit is referring to. I don't think it's that simple because of incompatibility with Docker Engine v29. OC, we can try:
|
The
HijackStream()method blocks hijacking if the stream is wrapped because it needs direct access to the raw transport stream for protocol upgrades. This PR changes how the library handles the response stream: when anUpgradeheader is set, theContent-Lengthheader is ignored, allowing the stream to be hijacked successfully, closes #34. This probably relates to and fixes #47 as well.@lanwen Thanks for already addressing the issue in TC Desktop. If I understood your comment correctly, you had something like this in mind. It would be great if you could take a quick look at the changes in
HttpConnectionto make sure I got it right. Thanks!