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.

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

net/http: missing error handling in serveContent #58556

Closed
agnivade opened this issue Feb 16, 2023 · 7 comments
Closed

net/http: missing error handling in serveContent #58556

agnivade opened this issue Feb 16, 2023 · 7 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.

Comments

@agnivade
Copy link
Contributor

What version of Go are you using (go version)?

$ go version
go version devel go1.21-a6ddb15f8f Tue Jan 24 17:58:12 2023 +0000 linux/amd64

What did you do?

I ran into this while debugging some failures while trying to stream a file from S3. We noticed problems where suddenly AWS started to return huge numbers of 403, but there were no error logs in our server. We eventually figured out the root cause to be a rate limiting issue, but we had to sift through CloudTrail logs to find that. The expectation was that it would appear in the server logs.

Digging a bit deeper while trying to reproduce locally, I narrowed it down to a missing error handler in:

go/src/net/http/fs.go

Lines 352 to 354 in 518889b

if r.Method != "HEAD" {
io.CopyN(w, sendContent, sendSize)
}

The problem is that if the stream breaks mid-way there is no indication anywhere as to what happened. The server will keep chugging along perfectly fine.

I have a 2 line CL already prepared for this which logs the error. But just wanted to create an issue for discussion in case this was intented in any ways.

I am happy to create a standalone reproducer for further proof, but I think it is pretty self-evident.

/cc @neild

@agnivade agnivade added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 16, 2023
@gopherbot
Copy link

Change https://go.dev/cl/469016 mentions this issue: net/http: add missing error handling in serveContent

@neild
Copy link
Contributor

neild commented Feb 17, 2023

Logging any error from io.CopyN doesn't seem right, given that the most likely cause of an error is going to be the client going away while writing the response body to it.

@agnivade
Copy link
Contributor Author

In our case, the cause was AWS throwing 403 and returning an xml response for an image file. I can write a reproducer, but it doesn't necessarily have to be the case of something stopping to write the response mid-way.

Anyways, just to clarify, you are okay with handling the error in some form, just not logging it? My only objective here is to be able to capture the error in some form or other, because otherwise there is absolutely no way for a server admin to know the reason of the error. They will see requests failing, but the reason will be unknown since the error is not captured in any way.

@neild
Copy link
Contributor

neild commented Mar 7, 2023

I'm open to some form of change here, but we shouldn't spam logs on the expected event of a client going away mid-response.

@agnivade
Copy link
Contributor Author

agnivade commented Mar 9, 2023

I commented on the CL but copying here as well: In our case, it wasn't the client, but AWS itself which sent a bad response and closed it. Therefore the problem was that it affected all clients, and there was nothing logged anywhere. So there was no way of knowing what was the exact error message thrown by AWS.

That is what I am trying to solve here. Happy to use any other solution that you recommend.

@agnivade
Copy link
Contributor Author

Friendly ping @neild

@neild
Copy link
Contributor

neild commented Jul 5, 2023

I don't know what, if any, change should be made here. I'm fairly certain that logging errors from writing to the response is not an improvement over the current behavior.

I realize this probably isn't a satisfactory response, but I'm afraid I don't have a better one at this time.

@agnivade agnivade closed this as completed Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants