reverseproxy: panic when reading from backend failed to propagate stream error

[WeidiDeng]

Fix 5951.

[francislavoie]

I'm confused. Is stdlib catching/recovering from the panic? That seems really strange to use a panic for control flow like this.

[WeidiDeng]

Both stdlib and http3 handle panic by aborting the stream. http3 used to have the problem that panicked handler will write 500 status instead of terminating the stream.

[WeidiDeng]

The stream has to be terminated. When we just return here, there are some cleanup to be done by stdlib which make the stream succeed without any problem.

[WeidiDeng]

@francislavoie stdlib does catch and recover panic. That special variable just stops logging.

[mholt]

Oh interesting -- so the stdlib is expecting a panic to properly clean up the stream that is terminated early. Otherwise it thinks it is successful. Bizarre, but yeah, let's see from the OP if it works!

[francislavoie]

Confirmed by reporter. LGTM!

[jcchavezs]

We run a nightly e2e test for a caddy plugin and ever since this PR got merged it started failing (see https://github.com/corazawaf/coraza-caddy/actions/runs/7002992650/job/19047820525#step:5:1) with the error:

{"level":"error","ts":1701077125.0196664,"logger":"http.handlers.reverse_proxy","msg":"aborting with incomplete response","upstream":"httpbin:8081","duration":0.000221322,"request":{"remote_ip":"172.18.0.1","remote_port":"45282","client_ip":"172.18.0.1","proto":"HTTP/1.1","method":"GET","host":"localhost:8080","uri":"/response-headers?pass=leak","headers":{"Accept-Encoding":["gzip"],"X-Forwarded-For":["172.18.0.1"],"X-Forwarded-Proto":["http"],"X-Forwarded-Host":["localhost:8080"],"User-Agent":["Go-http-client/1.1"],"Coraza-E2e":["ok"]}},"error":"short write"}

Any advice?

[WeidiDeng]

@jcchavezs From the error, it seems that your response writer doesn't return an error and the number of bytes written is less than the data passed. I have found one part that may cause this issue.

According to io.Writer documentation, you must return an error here, or not actually write, just return len(b). And you'll have to flush the underlying response writer as well to flush the response code.