Skip to content

fix: avoid hanging on 101 switching protocol responses#2518

Open
arpitjain099 wants to merge 1 commit into
projectdiscovery:devfrom
arpitjain099:fix/websocket-switching-protocol-hang
Open

fix: avoid hanging on 101 switching protocol responses#2518
arpitjain099 wants to merge 1 commit into
projectdiscovery:devfrom
arpitjain099:fix/websocket-switching-protocol-hang

Conversation

@arpitjain099

@arpitjain099 arpitjain099 commented Jun 18, 2026

Copy link
Copy Markdown

This fixes a hang when a target returns 101 Switching Protocols and keeps the upgraded connection open.\n\nThe request path still installed a body-drain defer when MaxResponseBodySizeToRead was set. For upgrade responses, that defer could block forever on io.Copy(io.Discard, body) even though we intentionally skip body reads for websocket-style responses.\n\nI now skip that drain defer for the same status codes where body reads are skipped (101, 304), and reuse that guard for the read branch too.\n\nI also added a regression test that stubs a 101 response with a body reader that never returns, and asserts Do returns instead of hanging.\n\nFixes #2517\n\n### Verification\n- go test ./common/httpx -run "TestDoSwitchingProtocolsDoesNotHang|TestSetCustomHeaders|TestParseCustomCookies|TestHTTP11DisablesRetryableHTTP2FallbackClient|TestDefaultProtocolKeepsRetryableHTTP2FallbackClient" -count=1\n- go build ./cmd/httpx\n- go run ./cmd/httpx -u "http://cediplast.myqnapcloud.com:8085" -stats -v -timeout 5 -nc

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a potential hanging issue when handling HTTP 101 Switching Protocols responses.
    • Optimized response body handling for specific HTTP status codes to improve reliability.

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ec00995-a786-4665-b764-4f3f22bfd678

📥 Commits

Reviewing files that changed from the base of the PR and between c59ccc5 and 5e0cb4e.

📒 Files selected for processing (2)
  • common/httpx/httpx.go
  • common/httpx/httpx_test.go

Walkthrough

HTTPX.Do is refactored to compute a single shouldSkipBodyRead flag for HTTP status codes 101 and 304, then gates both the deferred body drain/discard+close and the io.ReadAll call on that flag. A new test injects a round tripper returning a 101 response with a permanently blocking body and asserts Do completes within 4 seconds.

Changes

Fix hang on 101 Switching Protocols / 304 Not Modified

Layer / File(s) Summary
shouldSkipBodyRead flag and body drain gate
common/httpx/httpx.go
Computes shouldSkipBodyRead for status codes 101 and 304 once; uses it to skip the deferred drain+close and the io.ReadAll call, replacing the previous duplicated inline status-code checks.
Regression test for 101 hang
common/httpx/httpx_test.go
Adds TestDoSwitchingProtocolsDoesNotHang with a switchingProtocolsRoundTripper that returns a 101 response backed by a permanently blocking reader; asserts ht.Do returns within 4 seconds.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A websocket knock, the server said "101!"
httpx once froze — now that nightmare is done.
A flag called shouldSkip says "read not required,"
No draining, no hanging, no goroutines mired.
The rabbit hops free, four seconds or less! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main fix: avoiding hangs when processing 101 Switching Protocol responses, which is the core issue addressed in the changeset.
Linked Issues check ✅ Passed The PR successfully addresses issue #2517 by fixing the hang that occurs with 101 Switching Protocols responses through refactored body-handling logic and added regression testing.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the 101 Switching Protocols hang issue: refactored httpx.Do body handling, added test with blocking body reader, and imported time package for timeout assertions.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant