-
-
Notifications
You must be signed in to change notification settings - Fork 11
feat: Split http handler into sub projects #44
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?
Conversation
add native http handler add WriteClosableStreamWrapper for native http handler move WriteClosableStream into base namespace add HijackStreamHelper for reflection (conflict with System member) remove namedPipeConnectTimeout from config and hardcoded to 10s instead of 100ms
modify TestFixture to support dind and different clients add dind for tls client tests to ci add parallel test add speed test check for com.docker.service in Docker_IsRunning too add 1s sleep in MonitorEventsFiltered_Succeeds test
remove http handler from base
|
@HofmeisterAn Can you please review this PR too? Otherwise feel free to close this PR. I'm using this for some months for now and no errors found. Splitting handler into different packages make sense in my opinion. With legacy custom http handler implementation, I'm still facing memory and CPU problems, so I will keep using my fork, because for me this is critical. |
|
Thanks for the heads-up. I haven't forgotten the PR. I started it but didn't finish because other things kept coming up, and it's a pretty big PR. I'll try to review it in the next few days. Sorry about that.
I agree 👍. |
No worries, take your time. I'm happy that you maintain this project. 👍🏻 |
HofmeisterAn
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.
I think I can share the first part already. It would be great if we could reduce the copied classes by using a shared project or directory. This would make the review easier and help me see which classes were simply moved. Then I can continue with the actual content.
src/Docker.DotNet.NPipe/Microsoft.Net.Http.Client/BufferedReadStream.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Andre Hofmeister <9199345+HofmeisterAn@users.noreply.github.com>
Co-authored-by: Andre Hofmeister <9199345+HofmeisterAn@users.noreply.github.com>
Co-authored-by: Andre Hofmeister <9199345+HofmeisterAn@users.noreply.github.com>
Co-authored-by: Andre Hofmeister <9199345+HofmeisterAn@users.noreply.github.com>
Co-authored-by: Andre Hofmeister <9199345+HofmeisterAn@users.noreply.github.com>
…gth/TestContainers.Docker.DotNet into split-handler
|
@bruegth Thanks for taking care of the suggestions so quickly. I'll review the remaining parts on Sunday when my family is out. |
|
TODOs:
|
b2bd8d7 to
3e7b93b
Compare
HofmeisterAn
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.
I think the sources look good. I have a few more questions, and could you please review the recent changes to see if they are OK for you? I will update the tests sometime over the weekend.
| nativeHandler.UseProxy = true; | ||
| nativeHandler.AllowAutoRedirect = true; | ||
| nativeHandler.MaxAutomaticRedirections = 20; | ||
| nativeHandler.Proxy = WebRequest.DefaultWebProxy; |
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.
question: Do we need the proxy configuration? I'm not sure if this is compatible with the already existing proxy "configurations":
DetermineProxyModeAndAddressLine(HttpRequestMessage)TunnelThroughProxyAsync(HttpRequestMessage, Stream, CancellationToken)
I think we should move this to a separate PR (and make sure it is aligned with the exiting parts).
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.
yes new PR for that will be good.
Also I think the default OS proxy settings also should be overwritten by some new configs in future.
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.
Okay, then we need to clean this up (also remove reflection).
Sure it's okay for me. Your fine-tuning looks good and makes sense. |
| @@ -0,0 +1,18 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <AssemblyName>Docker.DotNet.Handler.Abstractions</AssemblyName> | |||
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.
Was the intention of new .Handler. package hierarchy to use it into e.g. npipe too?
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.
I'm not sure if I understood the question correctly. There will likely be more implementations in the future, for example for SSH or gRPC. My idea was to put everything into the handler project that these implementations need to fulfill.
|
I think we're almost done. Most tests passed, only 3/4 fail for the native implementation, which we need to check. |
HofmeisterAn
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 pretty good. Almost done I think.
| nativeHandler.UseProxy = true; | ||
| nativeHandler.AllowAutoRedirect = true; | ||
| nativeHandler.MaxAutomaticRedirections = 20; | ||
| nativeHandler.Proxy = WebRequest.DefaultWebProxy; |
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.
Okay, then we need to clean this up (also remove reflection).
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.
We need to update the README once more.
Project changes:
API changes:
Test changes:
FollowUp: