search

elazarl / goproxy

An HTTP proxy library for Go
BSD 3-Clause "New" or "Revised" License
5.89k stars 1.07k forks source link

send Connection: close header to client only when needed #514

Closed jakecoffman closed 6 months ago

jakecoffman commented 8 months ago

This fixes the inability to reuse connections by only setting the Connection: close header when the client or remote server requests it.

This also pulls in changes by @hmarr in #333 to explicitly terminate the connection instead of relying on an EOF from the client.

jeffwidman commented 6 months ago

@elazarl Sorry to bother, but I noticed this one's been floating for several months... I know the :dependabot: team currently uses a fork of Goproxy incorporating the change in #333 (which this PR includes) internally so it's already seen some prod traffic with no issues.

Any chance you could take a look? I'm sure they'd like to switch back to using upstream instead of a fork.

jakecoffman commented 6 months ago

We actually found this change might be problematic and we're testing without it to compare. So hold off on merging for now. Thanks @jeffwidman!

jeffwidman commented 6 months ago

Oh interesting. Since this PR includes two changes, are you observing the problem with #333 or with the other change in this PR?

elazarl commented 6 months ago

Let me know, and I'll merge whatever you deem helpful.

Thanks!

On Sat, Dec 16, 2023, 18:39 Jeff Widman @.***> wrote:

Oh interesting. Since this PR includes two changes, are you observing the problem with #333 https://github.com/elazarl/goproxy/pull/333 or with the other change in this PR?

— Reply to this email directly, view it on GitHub https://github.com/elazarl/goproxy/pull/514#issuecomment-1858863365, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAB7RIW6GXFORCCATK6LXDTYJXFDLAVCNFSM6AAAAAA6EDHJIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJYHA3DGMZWGU . You are receiving this because you were mentioned.Message ID: @.***>

jakecoffman commented 6 months ago

The problem is the change in #333. We observed write: connection reset by peer when doing Bundler updates which went away when we removed those lines in #333.

Now that we're running without that change we're seeing less timeouts.

I think there are some changes in this PR that we might still want, like not setting the Close header after each request, but I'd like to run it in Dependabot production for a bit to ensure it's good first.

You can close #333!

jeffwidman commented 2 months ago

I think there are some changes in this PR that we might still want, like not setting the Close header after each request, but I'd like to run it in Dependabot production for a bit to ensure it's good first.

@jakecoffman what's the word on these additional changes? Are any of them worth upstreaming?