net/http: missing error handling in serveContent

[agnivade]

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

[gopherbot]

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

[neild]

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]

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]

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]

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]

Friendly ping @neild

[neild]

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.