Panic on aborted requests to properly close the connection

[tonybart1337]

What does this PR do?

Fixes this issue

Might also fix this one. I tested it on HTTP/2 as well

Motivation

It breaks caching on our servers and many other things because we receive truncated data

Additional Notes

Client sends a HTTP/1.1 request to the server and downloads a file which is dynamically generated and returned in chunks, but sometimes the connection might abort midway and instead of aborting the connection traefik sends zero length chunk finalizing the request and the client ends up receiving a truncated response with no way of knowing whether it received the full file or not.

[rtribotte]

Hello @tonybart1337,

Thanks for investigating this and opening this PR!

I don't think raising a panic is the best solution in that case.
I would either return an InternalServerError if the response has not yet been started to be written to the client, or, when the response has indeed already started to be sent to the client (verifiable by wrapping the response writer), grab the connection and close it.

Would you be incline to rework the PR in that direction?
Thanks!

[tonybart1337]

Hi @rtribotte.

Thank you for taking your time on this PR.
I'm not a golang guru, but if you share a few hints I will gladly commit that into the PR.

I think it only lacks a few lines of code and this is a pretty serious bug

[rtribotte]

Hello @tonybart1337,

It is up to you, but the simplest way would that we push review commits to show the approach I was thinking of, it is always easier to demonstrate directly with code, WDYT?
Notably, wrapping the responseWriter could a bit challenging.

[rtribotte]

@tonybart1337 I can open a PR on your branch to demonstrate the approach, but the changes must be first rebased on branch v2.11, as this is a bug fix. Thanks!

[tonybart1337]

I'll share what I come up with in a few hours today

Looks like github button didn't rebase properly, I'll force push the fix as well

[tonybart1337]

@rtribotte rebased + implemented the wrapper

still not sure on how to properly close the connection, doesn't panic already handle it for us?

[rtribotte]

@tonybart1337 Thanks!

I think panics should be reserved for cases where the state is unexpected or irrecoverable, and here it seems possible to properly handle the response or close the connection.
I'll open a PR on your branch to propose adjustments.

[tonybart1337]

@rtribotte please lmk if I can help somehow, I totally agree that panics should be used as the last resort, but since it gets the job done and closes the connection properly, maybe we should merge it to fix the issue and then you can implement a proper solution without a panic?

this issue can lead to serious consequences like caching invalid data and serving it to the end users, or serving incomplete data while the end users will have no idea why this happened, or even worse if they have no means to check the validity of the data received

in most cases I believe that users will get an immediate error when they try to decode their binary data, like an image, for example, or maybe most users don't use "transfer-encoding: chunked" at all, otherwise I don't know how to explain why this is not fixed yet

[rtribotte]

Hello @tonybart1337,

Actually, I think you are right about reflecting the panic, this let the net/http server handle the response abortion.
I have opened tonybart1337#1 for review.
Could please address it and also add tests?
Thanks!

[rtribotte]

Thanks @tonybart1337 for merging the PR, we will leave the waiting for fixes label until the unit tests are fixed and a new one is introduced for this new behavior.

[kevinpollet]

Hello @tonybart1337,

Are you still willing to work on this pull request and address @rtribotte's changes, or do you want us to push some commits?

[tonybart1337]

Hi @kevinpollet .

If you could implement the tests yourselves that would be great, I don't have a lot of spare time at the moment 😞

[kevinpollet]

Thanks 👍

[rtribotte]

Thanks!