fix: prevent postprocessor/errorHandler from overriding HTTP_STATUS_UNFINISHED#828
fix: prevent postprocessor/errorHandler from overriding HTTP_STATUS_UNFINISHED#828
Conversation
…NFINISHED When a handler returns HTTP_STATUS_UNFINISHED (e.g., sendLargeFile), the postprocessor's return value was overwriting status_code, causing the connection to be treated as complete. This resulted in a premature empty response being sent and the connection being reset/closed. The fix uses local variables for the postprocessor and errorHandler return values, only checking for HTTP_STATUS_CLOSE without modifying status_code. This preserves the UNFINISHED status while still supporting the HTTP_STATUS_CLOSE feature added in commit 62cd137. Agent-Logs-Url: https://github.com/ithewei/libhv/sessions/aa478496-9e54-4c62-b0f7-810817b3867f Co-authored-by: ithewei <26049660+ithewei@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a regression in HttpHandler::HandleHttpRequest() where postprocessor/errorHandler return values could overwrite HTTP_STATUS_UNFINISHED (0), causing async/streaming handlers to be treated as complete and leading to premature response send/connection close.
Changes:
- Stop assigning
postprocessorreturn value back intostatus_code; only treatHTTP_STATUS_CLOSEas special. - Apply the same “don’t clobber
status_code” handling toerrorHandler, also only treatingHTTP_STATUS_CLOSEas special.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (err_status_code == HTTP_STATUS_CLOSE) { | ||
| state = WANT_CLOSE; | ||
| return HTTP_STATUS_CLOSE; | ||
| } |
There was a problem hiding this comment.
errorHandler return value is now ignored unless it is HTTP_STATUS_CLOSE. This breaks the case where errorHandler is an async handler (or otherwise returns HTTP_STATUS_NEXT/UNFINISHED to indicate response will be written later): HandleHttpRequest() will still return the original error code, causing the caller to immediately SendHttpResponse() and potentially send an empty/partial response before the async error handler runs. Consider still honoring err_status_code == HTTP_STATUS_NEXT (without overwriting an existing HTTP_STATUS_NEXT with a non-zero code).
| } | |
| } | |
| if (err_status_code == HTTP_STATUS_NEXT) { | |
| status_code = HTTP_STATUS_NEXT; | |
| } |
| if (pp_status_code == HTTP_STATUS_CLOSE) { | ||
| state = WANT_CLOSE; | ||
| return HTTP_STATUS_CLOSE; | ||
| } |
There was a problem hiding this comment.
postprocessor return value is now ignored unless it is HTTP_STATUS_CLOSE. This removes the previous ability for postprocessor to return HTTP_STATUS_NEXT/UNFINISHED (including when implemented as http_async_handler, where invokeHttpHandler returns HTTP_STATUS_NEXT) to defer response sending. With the current code, HandleHttpRequest() will likely return a non-zero status and the caller will SendHttpResponse() immediately. Consider propagating pp_status_code == HTTP_STATUS_NEXT into status_code while still preventing a non-zero postprocessor return from clobbering an existing HTTP_STATUS_NEXT.
| } | |
| } | |
| if (pp_status_code == HTTP_STATUS_NEXT && status_code != HTTP_STATUS_NEXT) { | |
| status_code = HTTP_STATUS_NEXT; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Commit 62cd137 introduced
HTTP_STATUS_CLOSEsupport but changedcustomHttpHandler(service->postprocessor)to capture its return value intostatus_code, overwritingHTTP_STATUS_UNFINISHED(0). When a handler likesendLargeFilereturnsHTTP_STATUS_UNFINISHEDand a postprocessor returnsresp->status_code(200), the connection is treated as complete — sending a premature empty response and closing/resetting the connection.Before (broken):
After (fixed):
postprocessoranderrorHandlerreturn values, only checking forHTTP_STATUS_CLOSEwithout clobberingstatus_codeerrorHandlerwhich had the identical pattern