Skip to content

martian: fix half-close propagation downstream when using http2 handler#870

Merged
mmatczuk merged 4 commits intomainfrom
mmt/h2_rst_fix
Jul 30, 2024
Merged

martian: fix half-close propagation downstream when using http2 handler#870
mmatczuk merged 4 commits intomainfrom
mmt/h2_rst_fix

Conversation

@mmatczuk
Copy link
Contributor

No description provided.

@mmatczuk mmatczuk requested a review from Choraden as a code owner July 26, 2024 12:54
@mmatczuk mmatczuk force-pushed the mmt/h2_rst_fix branch 3 times, most recently from 08509be to 9a0649a Compare July 26, 2024 13:06
errors.Is(err, syscall.ECONNABORTED) ||
errors.Is(err, syscall.ECONNRESET) {
errors.Is(err, syscall.ECONNRESET) ||
errors.Is(err, errClosedBody) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! I've seen a lot of these recently.

)

//go:linkname errClosedBody golang.org/x/net/http2.errClosedBody
var errClosedBody error
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that file name too general?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I called it h2Err... but reverted to that due to linter

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I was thinking of some h2error.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go:linkname moved to errors.go and renamed to h2ErrClosedBody.


func (w writeFlusher) Write(p []byte) (n int, err error) {
n, err = w.rw.Write(p)
func makeH2Writer(rw http.ResponseWriter, rc *http.ResponseController, req *http.Request) *h2Writer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Return a pointer? Previous flusher was a copy, and the methods receive a copy too, so I'm double checking if that's on purpose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, that's too fast generation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

.golangci.yml Outdated
Comment on lines +144 to +146
- linters:
- revive
source: '_ "unsafe"'
Copy link
Contributor

Choose a reason for hiding this comment

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

grep for already existing comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped that commit.

mmatczuk added 3 commits July 29, 2024 12:42
Use http2.Response.Close() to send RST_STREAM frame.
Reimplement writeFlusher as h2Writer.

This is to be tested in Sauce Connect due to lack of testing infrastructure for http2 at the moment.
I added #869 to fix that.
Copy link
Contributor

@Choraden Choraden left a comment

Choose a reason for hiding this comment

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

LGTM

@mmatczuk mmatczuk merged commit 0bc9b9c into main Jul 30, 2024
@mmatczuk mmatczuk deleted the mmt/h2_rst_fix branch July 30, 2024 13:28
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