-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix Randomly Broken Responses when Sending a File #9701
Conversation
Thank you @dl-eric! I dig the detail and unit tests! |
if (this.resp) |resp| { | ||
if (this.flags.is_waiting_for_request_body) { | ||
this.flags.is_waiting_for_request_body = false; | ||
resp.clearOnData(); | ||
} | ||
resp.endWithoutBody(closeConnection); | ||
resp.endWithoutBody(closeConnection, sendCrlf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR but a good cleanup would be to have a struct so we could have
resp.endWithoutBody({ .closeConnection = closeConnection, .sendCrlf = sendCrlf })
to avoid the boolean trap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG assuming test run passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this
While I think the fix is correct, I think we need to enable TCP_CORK to mimic libc behavior for headers/trailers or else it will be an extra network send (which will also make handling the lifetime of this more tricky). I think we should add a method to HttpResponse.h
which also looks at the status enum in here:
bun/packages/bun-uws/src/HttpResponseData.h
Lines 70 to 74 in 00b1561
HTTP_STATUS_CALLED = 1, // used | |
HTTP_WRITE_CALLED = 2, // used | |
HTTP_END_CALLED = 4, // used | |
HTTP_RESPONSE_PENDING = 8, // used | |
HTTP_CONNECTION_CLOSE = 16 // used |
It would also be great to try this for both SSL and non-SSL versions of the server, just to ensure we don't end up missing the CRLF when we need to keep it
Hey @Jarred-Sumner, thanks for your review! To clarify, do you mean we should cork the entire http response (headers + sendfile)? Right now I believe we send the headers corked, but the sendfile() bit is unoptimized. I agree that we should have lifetime hooks in HttpResponse/Data.h to dictate corking/uncorking responses such as these, and I'm interested in tackling the issue in a separate PR. Basically set TCP_CORK- send headers, send file - unset TCP_CORK. However, I see this PR as addressing correctness (without regression in perf) rather than optimization. Do you mind if I do the TCP_CORK logic for my next PR? I will also publish any perf improvements. Meanwhile, I've added an SSL flavor to the test - although, I noticed when SSL is enabled, we don't actually go through the sendfile() path, so the test isn't really giving us anything (yet). I'd be happy to add |
Thank you for this, we ended up fixing this and a number of other bugs in |
What does this PR do?
This PR fixes an issue where Bun will send an extra CRLF (/r/n) after sending a file for HTTP Servers. This extra CRLF is (1) not defined in the HTTP protocol and (2) can fail entire responses, depending on how the client handles these extra characters.
How did you verify your code works?
Wrote a test that fails without my change and passes with my change. It relies on a large volume of requests to trigger the bug, so I'm not sure how deterministic it is. But at least my machine was able to hit the bug every single test run on both debug and release builds.
Example test failure:
bun-debug test test-file-name.test
)