fix: do not prohibit HTTP/2 upstream on transient 429/503 - #2458
Open
2outside wants to merge 1 commit into
Open
Conversation
_dns_client_http2_process_stream_one() sets server_info->prohibit on any non-200 HTTP status. For 429 (Too Many Requests) and 503 (Service Unavailable) this is disproportionate: those codes are backpressure for the individual request, not a statement that the upstream is unavailable. Prohibiting the server calls _dns_client_shutdown_socket() and removes it from rotation for prohibit_time seconds, so one 429 tears down the shared HTTP/2 connection and excludes an otherwise healthy upstream. It is also self-reinforcing: the connection is re-established and the next burst of queries runs into the same server-side limit again. Fail only the affected query, which the caller already retries, and leave the connection and the server in rotation. Other status codes keep the previous behaviour. Tested against a local nginx DoH endpoint configured with limit_req and limit_req_status 429. After a burst of 30 concurrent queries the unpatched build logs "not alive, prohibit" and needs several seconds before the next query succeeds; the patched build never prohibits the server and the next query succeeds immediately.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the first item discussed in #2457.
Problem
_dns_client_http2_process_stream_one()setsserver_info->prohibit = 1for anynon-200 HTTP status:
For
429 Too Many Requestsand503 Service Unavailablethat is disproportionate.Those codes are backpressure aimed at the individual request; they do not say the
upstream is unavailable.
Following
prohibitinto_dns_client_check_server_prohibit()(
src/dns_client/dns_client.c), the flag causes_dns_client_shutdown_socket()to run,so the shared HTTP/2 connection is torn down, and the server is skipped until
prohibit_timeelapses. It is also self-reinforcing: the connection is re-establishedand the next burst of queries runs into the same server-side limit again.
Some public DoH providers apply a per-connection request budget and answer
429for theexcess. Because SmartDNS multiplexes all queries for one upstream over a single
connection, a normal burst — a batch of domains resolved at once, each producing A and
AAAA — can exceed it even when the average query rate is very low. On my routers one
upstream received roughly half the queries the other group members did, purely because it
kept cycling in and out of the prohibit state.
Change
Fail only the affected query for
429/503and leave the connection and the server inrotation. The caller already retries the query, and other members of the group answer in
the meantime. Every other status code keeps the previous behaviour.
Testing
Local nginx serving
/dns-queryover HTTP/2 with a self-signed certificate, proxying toa real upstream so responses are valid, and configured to produce the failure mode:
SmartDNS was configured with that endpoint as its only upstream, then hit with a burst of
30 concurrent queries. Recovery was measured as the time until the next query returned
NOERROR.not alive, prohibitloggedRepeated three times with the same outcome for the
prohibitcount. The measuredrecovery delay depends on which retry pass the query lands in (
prohibit_timeis 60 onthe first pass and 5 on the second), so the wait can be considerably longer than the 4s
observed here; with several upstreams in a group the practical effect is that a healthy
provider stops being used for that whole window.
Build is clean with no new warnings.
Not included
The concurrency/rate limiting discussed in #2457 is deliberately left out. I prototyped a
token-bucket pacer for the send path and measured it: pacing delays queries past
DNS_QUERY_TIMEOUT(500ms), which makes them time out and retry, so the total number ofrequests — and of
429responses — went up rather than down. That approach does not workfor a protocol whose own timeout is shorter than the burst it would need to absorb, so it
is not proposed here.