Skip to content

Conversation

@bruegth
Copy link

@bruegth bruegth commented Aug 26, 2025

  • Adds an option to use native dotnet http handler for http(s)
  • Extends tests for testing of different clients
  • Adds DockerInDocker services to CI for testing http/https
  • Adds performance tests

In high load scenarios (many calls to docker daemon) the custom http handler seems to consume a lot sockets, memory and CPU time.
For these scenarios this new option uses the native http handler, which works with socket pools and consumes less memory and CPU time. But this http native handler do not supports 'npipe' or 'unix' socket connections.

@bruegth bruegth force-pushed the standard-http-handler branch from 2b86300 to 73203b9 Compare August 27, 2025 10:47
@bruegth
Copy link
Author

bruegth commented Aug 27, 2025

@HofmeisterAn This way to test different DockerClients maybe helps for SSH feature too.

@bruegth
Copy link
Author

bruegth commented Aug 28, 2025

@HofmeisterAn added an performance test:
https://github.com/bruegth/TestContainers.Docker.DotNet/blob/91898b68f636fcd4060334a730d42ae6a7901580/test/Docker.DotNet.Tests/IContainerOperationsTests.cs#L135

But this fails locally for ManagedHttpHandler when thread count greater > ~60 (depends on hardware):
image

Also a have to raise connect timeout for namedpipe otherwise this tests fails too.
There is an issue here: dotnet#700

@bruegth
Copy link
Author

bruegth commented Aug 28, 2025

added "speedtest":
image

@HofmeisterAn
Copy link
Collaborator

Thanks for the contributions and effort! Could you share a bit more about why you chose to implement the native HTTP handler? I understand that using built-in classes is generally the better approach and something we should aim for instead of custom implementations, but I am curious if there are other reasons.

This PR touches some critical parts. Since the DockerClient ctor is already quite complex, making changes is not trivial, and the additional if-else checks and type handling make it even harder to follow. I will need a few days to review it carefully.

I really appreciate the extra tests, they are very helpful.

@bruegth
Copy link
Author

bruegth commented Sep 8, 2025

I'm facing still some strange CPU, Mem, SocketsLeak issues.

Sockets (https/managed over long time 1 day):
image

Sockets (https/native over short time 1h):
image

CPU/MEM with (https/managed some seconds):
image

CPU/MEM with (https/native some seconds):
image

@bruegth
Copy link
Author

bruegth commented Sep 9, 2025

@HofmeisterAn added memory output to new tests:
image

image

@bruegth
Copy link
Author

bruegth commented Sep 9, 2025

image

@bruegth
Copy link
Author

bruegth commented Sep 9, 2025

and finally cpu ticks:
image

@HofmeisterAn
Copy link
Collaborator

Could you quickly elaborate on what this PR is about? Does it address a specific issue? What changes are included?

Try to use standard dotnet http handler instead of custom one.

Are there any other changes besides that (apart from the test)?

I like the idea of testing all client configurations, but I don't think it'll work OOB (unless the endpoints are available). We'll probably need to make it configurable and adjust the CI pipeline (agent configuration).

@bruegth
Copy link
Author

bruegth commented Sep 23, 2025

Could you quickly elaborate on what this PR is about? Does it address a specific issue? What changes are included?

I updated my first comment.

And I need more time to fix test actions with 'dind', also maybe it will be good to test https client with self-signed certificates too.

* configure daemons and clients to match github and local test environments
* uses runners temp for certs
* checkout in different path
* reduce performance test asserts
* let task some time to start monitoring
@bruegth
Copy link
Author

bruegth commented Sep 23, 2025

@HofmeisterAn
I added 'dind' services to CI to test http+https.
Unfortunately this makes the TestFixture class very complicated, because of the combination of different daemons in relationship to the clients and local/github test environments.

Also the new parallel test for the 'ManagedHttps' client fails, and currently I don't know why, but I think this not related to the test: see here: https://github.com/bruegth/TestContainers.Docker.DotNet/actions/runs/17950135102/job/51046871489#step:9:18

@bruegth
Copy link
Author

bruegth commented Sep 24, 2025

Maybe related to dotnet/runtime#107051?

@HofmeisterAn
Copy link
Collaborator

I haven't had the time to look closely at the changes yet, but they do seem quite complex. Maybe it makes sense to split them into smaller chunks. I'll take a closer look in the next few days. BTW we use a similar setup/configuration in Testcontainers to test the SSL support/implementation, maybe that helps (I assume in general it should work).

You find the fixtures (DockerMTls, DockerTlsFixture, OpenSsl1_1_1Fixture, OpenSsl3_1Fixture, ProtectDockerDaemonSocket) here, and the corresponding tests here.

@bruegth
Copy link
Author

bruegth commented Oct 1, 2025

I haven't had the time to look closely at the changes yet, but they do seem quite complex. Maybe it makes sense to split them into smaller chunks. I'll take a closer look in the next few days. BTW we use a similar setup/configuration in Testcontainers to test the SSL support/implementation, maybe that helps (I assume in general it should work).

You find the fixtures (DockerMTls, DockerTlsFixture, OpenSsl1_1_1Fixture, OpenSsl3_1Fixture, ProtectDockerDaemonSocket) here, and the corresponding tests here.

I see, but to test with different fixtures which creates docker dind containers add the dependency to testcontainers to this project?

Also then we need to write same test for each fixture again, so I think my solution with

    [Theory]
    [MemberData(nameof(GetDockerClientTypes))]

is smarter because the tests only written once and it shows up a failure in client and not in test.

If you wish I will create an other PR with only adds tests for clients and another one to add the native handler, but before that I need to know if you accept this PR in common.

@HofmeisterAn
Copy link
Collaborator

I see, but to test with different fixtures which creates docker dind containers add the dependency to testcontainers to this project?

I meant this as a reference to a working dind setup running on GH-hosted runners.

If you wish I will create an other PR with only adds tests for clients and another one to add the native handler, but before that I need to know if you accept this PR in common.

Using built-in classes is great, and moving further in that direction sounds good 👍. Proper abstractions would really help (maybe start with DI, then add more handlers), especially if we support more protocols in the future (like SSH). WDYT?

My only concern is that too many if-else blocks make the code harder to maintain. Also, worth keeping in mind the library wasn't originally designed for many of the things it's doing now.

@bruegth
Copy link
Author

bruegth commented Oct 4, 2025

Using built-in classes is great, and moving further in that direction sounds good 👍. Proper abstractions would really help (maybe start with DI, then add more handlers), especially if we support more protocols in the future (like SSH). WDYT?

@HofmeisterAn Please take an look into: #44

But this solution uses reflection, which makes it difficult for AOT in future.

@bruegth
Copy link
Author

bruegth commented Oct 10, 2025

@HofmeisterAn Prefer this PR #44 in flavor, so closing this one.

@bruegth bruegth closed this Oct 10, 2025
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.

2 participants