web: do not send content on HTTPError(204) #3361
Conversation
Change `write_error()` to not send body if status code in (204, 304) or (100 <= status code < 200) - refactored into `RequestHandler._should_not_send_content(status_code)` Change `get_arguments()` to raise `ValueError` if `strip` is not boolean - as opposed to `AssertionError`. Fixes tornadoweb#3360
There was a problem hiding this comment.
One other thing to consider is that raise HTTPError(204) will be logged as a warning in log_exception. HTTPError was meant to be used for errors, not just a shorthand for "set status and return". We have another way to do that: self.set_status(204); raise tornado.web.Finish().
| @@ -1319,6 +1323,8 @@ def write_error(self, status_code: int, **kwargs: Any) -> None: | |||
| for line in traceback.format_exception(*kwargs["exc_info"]): | |||
| self.write(line) | |||
| self.finish() | |||
| elif self._should_not_send_content(status_code): | |||
There was a problem hiding this comment.
This should be the first branch in the if: even in serve_traceback mode, it's inappropriate to send it in a response to 204/304.
| @@ -466,7 +466,8 @@ def get_arguments(self, name: str, strip: bool = True) -> List[str]: | |||
| # Make sure `get_arguments` isn't accidentally being called with a | |||
| # positional argument that's assumed to be a default (like in | |||
| # `get_argument`.) | |||
| assert isinstance(strip, bool) | |||
| if not isinstance(strip, bool): | |||
There was a problem hiding this comment.
I mostly agree with getting rid of the assert statement, but this seems like a case where it's warranted - if it's relevant it'll fire all the time so you'll see it in development (or even better, a type checker can catch it statically), and then you can disable the assertion in production with -O
Change
write_error()to not send body ifstatus code in (204, 304) or (100 <= status code < 200),
refactored into
RequestHandler._should_not_send_content(status_code)Change
get_arguments()to raiseValueErrorifstripis not boolean, as opposed toAssertionError.Fixes #3360