Fix logging of incorrect status codes for disconnected requests (#12580)

The status code of requests must always be set, regardless of client
disconnection, otherwise they will always be logged as 200!.

Broken for `respond_with_json` in
f48792eec4.
Broken for `respond_with_json_bytes` in
3e58ce72b4.
Broken for `respond_with_html_bytes` in
ea26e9a98b.

Signed-off-by: Sean Quah <seanq@element.io>
This commit is contained in:
Sean Quah 2022-04-28 16:49:50 +01:00 committed by GitHub
parent 629aa51743
commit 0b684b59e5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 36 additions and 5 deletions

1
changelog.d/12580.bugfix Normal file
View file

@ -0,0 +1 @@
Fix a long standing bug where status codes would almost always get logged as 200!, irrespective of the actual status code, when clients disconnect before a request has finished processing.

View file

@ -683,6 +683,9 @@ def respond_with_json(
Returns:
twisted.web.server.NOT_DONE_YET if the request is still active.
"""
# The response code must always be set, for logging purposes.
request.setResponseCode(code)
# could alternatively use request.notifyFinish() and flip a flag when
# the Deferred fires, but since the flag is RIGHT THERE it seems like
# a waste.
@ -697,7 +700,6 @@ def respond_with_json(
else:
encoder = _encode_json_bytes
request.setResponseCode(code)
request.setHeader(b"Content-Type", b"application/json")
request.setHeader(b"Cache-Control", b"no-cache, no-store, must-revalidate")
@ -728,13 +730,15 @@ def respond_with_json_bytes(
Returns:
twisted.web.server.NOT_DONE_YET if the request is still active.
"""
# The response code must always be set, for logging purposes.
request.setResponseCode(code)
if request._disconnected:
logger.warning(
"Not sending response to request %s, already disconnected.", request
)
return None
request.setResponseCode(code)
request.setHeader(b"Content-Type", b"application/json")
request.setHeader(b"Content-Length", b"%d" % (len(json_bytes),))
request.setHeader(b"Cache-Control", b"no-cache, no-store, must-revalidate")
@ -840,6 +844,9 @@ def respond_with_html_bytes(request: Request, code: int, html_bytes: bytes) -> N
code: The HTTP response code.
html_bytes: The HTML bytes to use as the response body.
"""
# The response code must always be set, for logging purposes.
request.setResponseCode(code)
# could alternatively use request.notifyFinish() and flip a flag when
# the Deferred fires, but since the flag is RIGHT THERE it seems like
# a waste.
@ -849,7 +856,6 @@ def respond_with_html_bytes(request: Request, code: int, html_bytes: bytes) -> N
)
return None
request.setResponseCode(code)
request.setHeader(b"Content-Type", b"text/html; charset=utf-8")
request.setHeader(b"Content-Length", b"%d" % (len(html_bytes),))

View file

@ -201,4 +201,16 @@ class CasHandlerTestCase(HomeserverTestCase):
def _mock_request():
"""Returns a mock which will stand in as a SynapseRequest"""
return Mock(spec=["getClientIP", "getHeader", "_disconnected"])
mock = Mock(
spec=[
"finish",
"getClientIP",
"getHeader",
"setHeader",
"setResponseCode",
"write",
]
)
# `_disconnected` musn't be another `Mock`, otherwise it will be truthy.
mock._disconnected = False
return mock

View file

@ -349,4 +349,16 @@ class SamlHandlerTestCase(HomeserverTestCase):
def _mock_request():
"""Returns a mock which will stand in as a SynapseRequest"""
return Mock(spec=["getClientIP", "getHeader", "_disconnected"])
mock = Mock(
spec=[
"finish",
"getClientIP",
"getHeader",
"setHeader",
"setResponseCode",
"write",
]
)
# `_disconnected` musn't be another `Mock`, otherwise it will be truthy.
mock._disconnected = False
return mock