Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jakecoffman
Copy link

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.

@jakecoffman @hmarr
send Connection: close header to client only when needed
294d463 
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 to explicitly terminate the connection instead of relying on an EOF from the client.

Co-authored-by: Harry Marr <harry.marr@gmail.com>
@jeffwidman jeffwidman mentioned this pull request Nov 6, 2023
Closed
@jeffwidman
Copy link
Contributor

@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
Copy link
Author

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
Copy link
Contributor

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

@elazarl
Copy link
Owner

elazarl commented Dec 16, 2023 via email

@jakecoffman
Copy link
Author

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!

@jakecoffman jakecoffman closed this Jan 2, 2024
@jeffwidman
Copy link
Contributor

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?

@ErikPelli ErikPelli mentioned this pull request Dec 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakecoffman @jeffwidman @elazarl