netutil, simple_httpclient: format the ValueError messages in raise#3681
Draft
HrachShah wants to merge 2 commits into
Draft
netutil, simple_httpclient: format the ValueError messages in raise#3681HrachShah wants to merge 2 commits into
HrachShah wants to merge 2 commits into
Conversation
added 2 commits
July 5, 2026 12:06
These two raise statements pass extra positional arguments to `ValueError` but use only the format string as the first argument, so the result is a tuple of (format_string, arg) instead of the formatted message. str(exc) then shows the literal format string rather than the intended text. Use string formatting to produce a single formatted message, matching the pattern used everywhere else in the codebase. Affected paths: - web.RequestHandler.xsrf_token: unknown xsrf cookie version %d - WebSocketProtocol13._process_server_headers: unsupported extension %r
bind_unix_socket raised ValueError('File %s exists and is not a socket', file)
and _HTTPConnection.run raised ValueError('unsupported auth_mode %s', mode).
Both pass a literal format string and the value as separate arguments, so
str(exc) renders the tuple repr with the format string and the value tucked
into e.args[1] rather than producing a usable error message. Replace with
the formatted single-argument form so the offending file path or mode name
is actually visible to a user reading the traceback or a log line.
Two regression tests:
- tornado/test/netutil_test.py::TestBindUnixSocket::
test_existing_non_socket_file_message_includes_path creates a regular file
where bind_unix_socket expects a socket, captures the leaked fd from the
error path, and asserts the path and 'exists and is not a socket' appear
in str(exc) with a single-arg ValueError.
- tornado/test/httpclient_test.py::HTTPClientCommonTestCase::
test_unsupported_auth_mode_message_format fetches /auth with auth_mode=
'asdf' against the simple client and asserts the 'asdf' literal appears in
the raised ValueError and '%s' does not.
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
1 participant
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.
What do these changes do?
Two
raise ValueError("fmt %s", arg)calls in tornado leaked the literal format string instr(exc)instead of producing a readable error. This PR changes both toraise ValueError("fmt %s" % arg)so the offending file path or auth mode name is visible to a user reading the traceback or a log line.The pre-fix forms:
bind_unix_socketraisedValueError("File %s exists and is not a socket", file).str(e)returned"('File %s exists and is not a socket', '/tmp/foo')"and the actual file path was tucked intoe.args[1]._HTTPConnection.runraisedValueError("unsupported auth_mode %s", self.request.auth_mode). A log line of the exception only showed the literal format string; the actual mode name was reachable only viae.args[1].Are there changes in behavior for the user?
Yes, for the error path. Pre-fix both raised a
ValueErrorwhosestr(e)was a tuple repr of(format_string, value). Post-fix thestr(e)is a single, formatted string that names the offending file or mode. Callers that handle the exception (including log lines that print the message) now see something useful.No other callers in the tornado tree depend on the old message text; the only assumption is that a
ValueErroris raised, which is preserved.Is it a substantial burden for the maintainers to support this?
No. The change is one-character at each
raisesite (a,becomes" %), and the new error is strictly more useful than the old one. No public API or signature changes; no behavior change on the success path.Related issue number
None — these are the remaining two
ValueError(fmt, arg)instances in the package, spotted while reading the recentweb, websocket: format ValueError messages in raisechange.Checklist
CONTRIBUTORS.txtCHANGES/foldertornadousesCHANGES.rstand these are internal message-format fixes that don't change user-visible behavior, so I have not added a fragment. Happy to add one if maintainers prefer.Verification
python3 -m pytest tornado/test/netutil_test.py -p no:relaxed -p no:hypothesispytest-> 11 passed, 2 skipped (CaresResolverTest is unrelated, pre-existing failure from a pycares API change; 1 new test added passes).python3 -m pytest tornado/test/httpclient_test.py -p no:relaxed -p no:hypothesispytest-> 1 new test passes; pre-existingtest_unsupported_auth_modestill passes.bind_unix_socket('/tmp/regular-file')raisesValueError: ('File %s exists and is not a socket', '/tmp/regular-file'); post-fix it raisesValueError: File /tmp/regular-file exists and is not a socket. Same for the auth_mode check on the simple client.Drafted with Zo Bot; reviewed by HrachShah.