Skip to content
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

Improve handling of errors from body.Close() #526

Merged
merged 2 commits into from
Nov 16, 2023
Merged

Improve handling of errors from body.Close() #526

merged 2 commits into from
Nov 16, 2023

Conversation

Choraden
Copy link
Contributor

No description provided.

@what-the-diff What The Diff
Copy link

what-the-diff bot commented Nov 14, 2023

PR Summary

  • Increased Testing for Connection Reuse
    A new test function TestProxyReuseConnection was introduced in the file e2e/tests/proxy_test.go. This enhancement helps verify that our proxy is accurately reusing connections.

  • Improved Handling of Close Body Operations
    The manner in which we close request and response bodies has been updated in files internal/martian/handler.go and internal/martian/proxy.go. Previously, closure of bodies was directly handled by calling Close(). This has been altered to instead use the new function mustCloseBody. This modification provides better error handling by logging and triggering http.ErrAbortHandler when errors occur during closure.

  • Additional Function for Close Body Operations
    To accommodate these improvements, a new function closeBody was introduced in internal/martian/proxy.go. This function is designed to efficiently manage and log errors while closing the body.

These changes ensure a higher degree of reliability and seamless operation of our application. Review of individual files for understanding the changes in detail is encouraged.

@Choraden Choraden linked an issue Nov 14, 2023 that may be closed by this pull request
martian: improve handling of round trip errors #486
Closed
mmatczuk
mmatczuk reviewed Nov 14, 2023
View reviewed changes
e2e/tests/proxy_test.go Outdated
Comment on lines 310 to 311
"http://wronghost/",
"http://differenthost",
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange host names

mmatczuk
mmatczuk reviewed Nov 14, 2023
View reviewed changes
internal/martian/proxy.go Outdated
@@ -728,7 +729,7 @@ func (p *Proxy) handle(ctx *Context, conn net.Conn, brw *bufio.ReadWriter) error
}
return errClose
}
defer req.Body.Close()
defer closeBody(req.Context(), req.Body, &retErr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Creative but wrong.

Use defer func...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why wrong?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not wrong but passing ptr to the return value is a form of hack.

Other thing is that we should return body close error only if there are no other errors.

IMO there should be defer req.Body.Close() and a manual body close by the end the way you handle files for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

mmatczuk
mmatczuk reviewed Nov 15, 2023
View reviewed changes
internal/martian/handler.go Outdated
@@ -164,7 +164,7 @@ func (p proxyHandler) handleConnectRequest(ctx *Context, rw http.ResponseWriter,
res = p.errorResponse(req, cerr)
p.warning(res.Header, cerr)
}
defer res.Body.Close()
defer mustCloseBody(res.Request.Context(), res.Body)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe handler does not need that because the server closes the body close and manages the connections i.e. it's out of our control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course!

mmatczuk
mmatczuk reviewed Nov 15, 2023
View reviewed changes
e2e/tests/proxy_test.go Outdated
"http://wronghost2",
} {
res := c.GET(h, func(req *http.Request) {
var body bytes.Buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you try to use io.Pipe for a body or a limit reader of 10MB reading zeros?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added io.Pipe with zeros to be read.

mmatczuk
mmatczuk reviewed Nov 15, 2023
View reviewed changes
e2e/tests/proxy_test.go Outdated
Comment on lines 310 to 311
"http://wronghost1",
"http://wronghost2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Host does not matter, you could as well have a plain loop.

@mmatczuk
Copy link
Contributor

Drop the last commit it does not belong here. Also it only logs in one place.

@mmatczuk
Copy link
Contributor

Otherwise lgtm.

@Choraden Choraden merged commit 9934b88 into main Nov 16, 2023
@Choraden Choraden deleted the hg/body_exhaust branch November 16, 2023 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmatczuk mmatczuk mmatczuk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

martian: improve handling of round trip errors
2 participants
@Choraden @mmatczuk