Skip redirect set lookup for non-3xx responses - #2301
Conversation
Redirect30xInterceptor.REDIRECT_STATUSES is a Set<Integer>, so the membership test in Interceptors.exitAfterIntercept autoboxed the int status code. HTTP status codes are all above 127 and therefore outside the range Integer.valueOf caches, so every response allocated a fresh Integer and hashed it, only for the answer to be false on the 2xx, 4xx and 5xx responses that make up almost all traffic. Guard the lookup with a 300..399 range check. The set is public and mutable, so it is deliberately still consulted rather than inlined as a switch over the five known codes: a caller that registered an extra 3xx status keeps having it honoured, and only a genuine redirect now pays for the boxing. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // Range check first: REDIRECT_STATUSES is a Set<Integer>, so contains(statusCode) boxed a fresh | ||
| // Integer on every response (status codes are outside Integer's valueOf cache). The set is public | ||
| // and mutable, so the lookup is kept rather than inlined as a switch, and a caller that registered | ||
| // an extra 3xx status still has it honoured. |
There was a problem hiding this comment.
Can we drop this one? It describes the change rather than the code, and once the old version is out of memory it just reads as noise. It is also four lines of prose for a one line predicate, where the rest of the file only comments invariants. If you want something here, one line does it: only a 3xx can be a redirect, so the range check keeps the boxed lookup off the common path.
There was a problem hiding this comment.
Dropped. It was describing the change rather than the code, and with the predicate moved into Redirect30xInterceptor the call site reads on its own, so there is nothing left worth saying there.
| // Integer on every response (status codes are outside Integer's valueOf cache). The set is public | ||
| // and mutable, so the lookup is kept rather than inlined as a switch, and a caller that registered | ||
| // an extra 3xx status still has it honoured. | ||
| if (statusCode >= 300 && statusCode < 400 && Redirect30xInterceptor.REDIRECT_STATUSES.contains(statusCode)) { |
There was a problem hiding this comment.
Netty already has this one. HttpStatusClass.REDIRECTION.contains(statusCode) takes an int and uses the same 300/400 bounds. We do not use HttpStatusClass anywhere else yet, so take it or leave it, the explicit range reads fine.
There was a problem hiding this comment.
Taken. REDIRECTION is built with 300/400 bounds and contains takes an int, so it keeps the boxing off non-3xx responses exactly as the open-coded range did, and it names what the bounds mean instead of leaving two magic numbers. First use of HttpStatusClass in the codebase, but that seems like a reason to start rather than not to.
| // Integer on every response (status codes are outside Integer's valueOf cache). The set is public | ||
| // and mutable, so the lookup is kept rather than inlined as a switch, and a caller that registered | ||
| // an extra 3xx status still has it honoured. | ||
| if (statusCode >= 300 && statusCode < 400 && Redirect30xInterceptor.REDIRECT_STATUSES.contains(statusCode)) { |
There was a problem hiding this comment.
Minor: the range now lives here while the set lives in Redirect30xInterceptor, so the knowledge is split across two classes. A static isRedirect(int) next to the set would keep it in one place and give you something to test. Fine to skip if you want the diff minimal.
There was a problem hiding this comment.
Done. Package-private isRedirect(int) next to the set, so Interceptors just asks the question and the range and the statuses stay together.
Added Redirect30xInterceptorTest for it. The case that earns the test is 304: in the 3xx class, not a followed redirect, so it has to be rejected exactly like a non-3xx status. Neither half of the predicate says that on its own.
Review feedback on AsyncHttpClient#2301. The range check lived in Interceptors while the statuses it guards live in Redirect30xInterceptor, splitting one decision across two classes. Move it into a package-private isRedirect(int) beside the set, so Interceptors reads as a question about redirects and the knowledge stays in one place. Use Netty's HttpStatusClass.REDIRECTION for the class check rather than an open-coded 300..400: it takes an int, so it still keeps the boxing lookup off non-3xx responses, and it names what the bounds mean. Drop the comment at the call site. It described the previous version of the code rather than the code, and the predicate now says what it does. The new test covers the part that is not obvious from either half on its own: a 3xx that is not a followed redirect, 304 above all, has to be rejected exactly like a non-3xx status. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Problem
Interceptors.exitAfterInterceptruns for every HTTP response and tested theredirect status like this:
REDIRECT_STATUSESis aSet<Integer>andstatusCodeis anint, so thecall autoboxes. Every HTTP status code is above 127 and therefore outside the
range
Integer.valueOfcaches, so this allocated a freshIntegerand hashedit on every response, only to answer
falsefor the 2xx / 4xx / 5xx responsesthat make up almost all traffic.
Change
Guard the lookup with a
300..399range check, so the boxing happens only foran actual 3xx response.
The set is left in place and still consulted, deliberately, rather than inlined
as a
switchover the five known codes:REDIRECT_STATUSESispublicand amutable
HashSet, so a caller may have registered an additional redirectstatus. Keeping the lookup behind a range check preserves that for any 3xx
addition while removing the per-response cost. Behaviour differs only for a
non-3xx code added to the set, which would not be a redirect status.
No public API change.
Scope
One line of logic plus a comment. Related per-response allocations found in the
same method (
responseHeaders.getAll(SET_COOKIE)allocates aLinkedListperresponse even with no
Set-Cookiepresent) are left for a separate PR.No new tests: there is no observable behaviour change, and the 3xx paths are
already covered by
Relative302Test,PerRequestRelative302Test,PostRedirectGetTest,RedirectBodyTest,HttpToHttpsRedirectTest,RedirectCredentialSecurityTest,RedirectConnectionUsageTest,Head302Test,StripAuthorizationOnRedirectHttpTestandws.RedirectTest(48 tests, allgreen on this branch).
Verification
mvnw clean verify- BUILD SUCCESS, 1371 tests, 0 failures, 0 errors,19 skipped. Error Prone, NullAway and Revapi all clean.
Caveat on the testing gate:
AGENTS.mdrequires the build to run on JDK 11 andno JDK 11 is installed on this machine, so it was run on JDK 17 (also in the
CI matrix). The JDK 11 leg of CI on this PR is the real gate.
Follows the same review pass as #2300.
Claude Code on behalf of @pavel-ptashyts
🤖 Generated with Claude Code