fix: -pr http11 flag ignored due to HTTP/2 fallback in retryablehttp-go#2419
fix: -pr http11 flag ignored due to HTTP/2 fallback in retryablehttp-go#2419SolariSystems wants to merge 4 commits intoprojectdiscovery:devfrom
Conversation
…lback Addresses projectdiscovery#2240 Signed-off-by: Mark Brush <solarisys2025@gmail.com>
Neo - PR Security ReviewNo security issues found Highlights
Comment |
…lback option retryablehttp-go Options does not have a DisableHTTP2Fallback field yet. Instead, override HTTPClient2 to point to the same HTTP/1.1 client after creation. This prevents the automatic HTTP/2 fallback in retryablehttp-go do.go when -pr http11 is set, without requiring upstream library changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaces a string protocol check with a constant and, when HTTP/1.1-only mode (HTTP11) is selected, makes the HTTP/2-fallback client reuse the primary client's transport to prevent silent HTTP/2 upgrades. Tests added to verify both HTTP11 and default behaviors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/httpx/httpx_test.go`:
- Around line 28-38: Update TestDefaultProtocolKeepsHTTP2Fallback to not only
assert ht.client.HTTPClient2 is non-nil but also assert it is a distinct
instance from ht.client.HTTPClient by adding a require.NotSame (or
require.NotEqual with pointer comparison) between ht.client.HTTPClient and
ht.client.HTTPClient2; keep the existing require.NotNil check and use the same
test helper names (TestDefaultProtocolKeepsHTTP2Fallback, New, Options,
ht.client.HTTPClient, ht.client.HTTPClient2) so the test ensures HTTP/2 fallback
remains separate from the main HTTPClient.
SolariSystems
changed the title
|
@Mzack9999 Fixed — the build failure was caused by referencing Instead, the fix now overrides if httpx.Options.Protocol == HTTP11 {
httpx.client.HTTPClient2 = httpx.client.HTTPClient
}This prevents retryablehttp-go's automatic HTTP/2 fallback without requiring upstream changes. Tests updated accordingly — both build and tests pass locally. |
…TP/2 enabled Ensures that default protocol keeps HTTPClient2 as a distinct HTTP/2 client, not accidentally sharing the same instance as HTTPClient.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/httpx/httpx_test.go`:
- Around line 16-26: The test TestHTTP11DisablesHTTP2Fallback calls New(&opts)
which may mutate the process-wide GODEBUG; to avoid leaking state restore the
original value around that call by saving os.LookupEnv("GODEBUG") (or use
t.Setenv if available) before calling New and defer restoring it after (or rely
on t.Setenv to automatically revert); update the test to capture the original
GODEBUG, set/restore it so New(Options{Protocol: HTTP11}) cannot leave GODEBUG
changed, leaving the rest of the assertions (require.Nil and require.Same)
unchanged and referencing the same New, Options, and Protocol/HTTP11 symbols.
Save and restore the GODEBUG env var around New() call to prevent state leaking into other tests when HTTP/1.1 mode sets http2client=0.
2 participants
Summary
Fixes #2240
Changes Made
Clean, minimal, and correct fix. The 3-line option change properly threads the HTTP/1.1 preference into retryablehttp-go's existing DisableHTTP2Fallback guard (verified at do.go:63 and client.go:119), the string-literal-to-constant cleanup is a welcome bonus, and the two unit tests directly verify the structural invariant that makes the fix work.
Verification
/claim #2240
Summary by CodeRabbit
Bug Fixes
Tests