Skip to content

Fix client cleanup after malformed HTTP responses - #3715

Closed
ryanchou1994 wants to merge 1 commit into
tornadoweb:masterfrom
ryanchou1994:fix/malformed-response-close
Closed

ryanchou1994 wants to merge 1 commit into
tornadoweb:masterfrom
ryanchou1994:fix/malformed-response-close

Conversation

@ryanchou1994

Copy link
Copy Markdown

When a response has a malformed start line or header, SimpleAsyncHTTPClient can wait for the request timeout even though the stream has already closed. For example, HTTP/1.1 301\r\n produces HTTPTimeoutError instead of reporting the failed connection promptly.

The parser can raise HTTPInputError before need_delegate_close is set. Set that flag in the client-side error handler so the existing finally block notifies the delegate and releases the request. HTTP parsing rules and the server-side 400 response remain unchanged; callers receive HTTPStreamClosedError for this failure instead of a later timeout.

Related to #2984, specifically the malformed-response timeout discussed there.

Five real-socket regression tests cover malformed start lines and headers, including after one or two 100 Continue responses, alongside valid-response controls. They also check callback counts, timer cleanup, and reuse of the single client slot. The four malformed cases fail against the original implementation and pass with this change.

Validation on macOS and Linux:

  • tox lint, docs and the complete py3 suite pass: 1,250 tests, with 56 existing skips on macOS and 54 on Linux.
  • The focused HTTP client/connection/server tests pass (19 tests).
  • Two earlier macOS runs hit existing timing-sensitive tests; both isolated reruns and the final full run passed without changing their timeouts or expectations.

Tests used local sockets only. Windows and optional curl/pycares/Twisted transports were not run locally.

Copilot AI lite review requested due to automatic review settings September 9, 2026 17:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ryanchou1994

Copy link
Copy Markdown
Author

Hi @bdarnell, friendly ping: the workflows on this PR are still waiting for approval to run. Happy to adjust anything if needed. Thanks!

@ryanchou1994

Copy link
Copy Markdown
Author

Closing this one: #3765 (merged 2026-10-06) includes the same fix in http1connection.py, setting need_delegate_close when the parse error happens on the client side before headers_received, so this PR is redundant now.

For what it's worth, the regression tests from this branch (MalformedResponseTestCase, 5 cases) pass unchanged on current master, and fail on master with that elif branch removed, so the hang from #2984 is covered. If a tests-only follow-up for these cases would be useful, I'm happy to open a small PR with just the test class. Thanks!

@bdarnell

bdarnell commented Oct 7, 2026

Copy link
Copy Markdown
Member

Thanks for the follow-up! And thanks for connecting this fix back to #2984. I think we probably don't need additional heavyweight tests for this two-line fix, but if there are specific cases that you think would benefit from dedicated coverage I'd consider a test-only PR to add them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants