Skip to content

refactor: Replace deprecated httputil.ClientConn with modern HTTP components, upgrade libraries #15

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

Closed
wants to merge 1 commit into from

Conversation

analytically
Copy link

This commit replaces the deprecated httputil.NewClientConn function with a custom implementation that uses the recommended http.Transport and http.Client from the standard library. The new implementation maintains all existing interfaces and should be fully compatible with existing code and tests.

…ponents, upgrade libraries

This commit replaces the deprecated httputil.NewClientConn function with a custom
implementation that uses the recommended http.Transport and http.Client from the
standard library. The new implementation maintains all existing interfaces and
should be fully compatible with existing code and tests.

Signed-off-by: Mathias Bogaert <[email protected]>
Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this implementation will actually work.

First, some context: I recall conversations about the usage of httputil and the only reason we still use it is because of it's Hijack() method. As you found out, http.Client does not have a Hijack() method. There is a Hijacker interface but that's only supported for Handler funcs really. You can see folks asking for a Hijack() func here: golang/go#28030

Immediate concerns looking at this is that it doesn't actually let us hijack. All you're doing here is taking in the net.Conn and passing it to the http.Transport which gets fed into http.Client. When you call Hijack() in the implementation here you pass back the net.Conn you fed into http.Transport. The problem with that is that you haven't actually freed the net.Conn from http.Client or http.Transport at all. You end up trying to share the net.Conn with http.Client and whoever else now thinks they've hijacked.

If you look at Hijack() from httputil: https://cs.opensource.google/go/go/+/refs/tags/go1.24.1:src/net/http/httputil/persist.go;l=65-73
You can see that they free themselves of the net.Conn before passing it back to the caller.

The Hijack here does not do that and AFAIK there isn't a way to do it with http.Client. Maybe there's a way to do it with the way you've hooked into http.Transport's DialerContext, but even that I'm a little bit sus'd about. If you look at the default DialerContext you'll see that it's doing a lot of stuff: https://cs.opensource.google/go/go/+/refs/tags/go1.24.1:src/net/dial.go;l=525-579


Ended up testing this locally to see if it maybe works, and it sadly does not. I set a pipeline and tried running it but everything hangs. The job is stuck in pending, resource checks get stuck, everything just hangs.


We could probably look at where we're using Hijack() within Concourse and try figuring out another way to accomplish our goals. I don't think we'll have any success trying to rip net.Conn away from http.Client or http.Transport.

@analytically
Copy link
Author

Sounds like this isn't something easy to solve as the test didn't cover the hijack functionality and it passes with this PR, making me assume my change worked. Let's park this for now as it's something that's working.

@taylorsilva
Copy link
Member

Closing this PR because I don't see a path forward with replacing httputil without a deeper overhaul in concourse/concourse.

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