process_request returning response runs connection handler

[Flowrey]

I'm using:

• Python: 3.12
• websockets: 12.0

When using the threading implementation like in this code:

import http
from websockets.sync.server import serve, ServerConnection, Request, Response
from websockets.http11 import datastructures

def health_check(websocket: ServerConnection, request: Request):
    if request.path == "/healthz":
        return Response(http.HTTPStatus.OK, "OK", datastructures.Headers([]), b"OK")
    else:
        return None


def echo(websocket):
    for message in websocket:
        websocket.send(message)

def main():
    with serve(echo, "localhost", 8765, process_request=health_check) as server:
        server.serve_forever()

main()

Any call to the /healthz endpoint works but raise a ConnectionCloseError:

connection handler failed
Traceback (most recent call last):
  File "/home/flowrey/Code/poc/venv/lib64/python3.12/site-packages/websockets/sync/server.py", line 499, in conn_handler
    handler(connection)
  File "/home/flowrey/Code/poc/server.py", line 13, in echo
    for message in websocket:
  File "/home/flowrey/Code/poc/venv/lib64/python3.12/site-packages/websockets/sync/connection.py", line 162, in __iter__
    yield self.recv()
          ^^^^^^^^^^^
  File "/home/flowrey/Code/poc/venv/lib64/python3.12/site-packages/websockets/sync/connection.py", line 201, in recv
    raise self.protocol.close_exc from self.recv_events_exc
websockets.exceptions.ConnectionClosedError: no close frame received or sent

[MauroAntonino]

I noticed that it is receiving empty bytes as a data response, and this causes the socket to close.

websockets/src/websockets/sync/connection.py

Lines 549 to 550 in e217458

	if data == b"":
	break

websockets/src/websockets/sync/connection.py

Lines 614 to 616 in e217458

	finally:
	# This isn't expected to raise an exception.
	self.close_socket()

Another thing is that when the status code is not 101, the state does not end up being OPEN, which then closes the connection as well.

websockets/src/websockets/server.py

Lines 541 to 547 in e217458

	if response.status_code == 101:
	assert self.state is CONNECTING
	self.state = OPEN
	else:
	self.send_eof()
	self.parser = self.discard()
	next(self.parser) # start coroutine

websockets/src/websockets/sync/server.py

Lines 143 to 146 in e217458

	if self.protocol.state is not OPEN:
	self.recv_events_thread.join(self.close_timeout)
	self.close_socket()
	self.recv_events_thread.join()

I have made this PR that solved this error for me, but I don't know if it broke tests or other functinalities
#1440

[aaugustin]

The change causes tests to hang -- because you removed the code paths that manages the end of a connection.

The issue looks legit but that's not the right fix.

[MauroAntonino]

I think I figured it out now. The code is handling all connections as WebSocket connections. When it is an HTTP connection and a request is made, it is supposed to close the connection, which differs from a WebSocket, where it is expected to keep the connection alive. I created a checker to verify if it is a WebSocket before handling it.

I have created this new PR with this change:
#1443

[aaugustin]

@MauroAntonino - the approach in your PR is generally correct; however it can be simplified to if connection.state is OPEN.

Also, we leak an open socket in that case. We should close it.

[aaugustin]

Rephrased title to reflect the fact that this issue also affects the new asyncio implementation

[aaugustin]

After further analysis, it turns out that the problem happens when returning a Response object in process_request or process_response that was initialized directly, as opposed to being created with self.respond(...).

In that case, handshake() could complete without opening the connection and without raising an exception. This broke an invariant on which websockets relied.

The fix for this problem actually belongs in the Sans-I/O layer. By setting protocol.handshake_exc when a non-101 HTTP response is effectively sent, as opposed to the point when it's created, we can guarantee the invariant again.

This makes the logic robust to (1) creating a response "manually" and (2) creating and not sending a response, which could cause the opposite problem.

[aaugustin]

Almost there... needs a test for the behavior change in the Sans-I/O layer.

[tpeacock19]

Hello, I'm using 13.1 and the below process_request for a health check and I'm still seeing these errors pop up in logs. Is there something I should be doing differently?

async def process_request(
    self, connection: websockets.asyncio.server.ServerConnection, request: Request
) -> Response | None:
    global HEALTH_PING

    if request.path == "/health":
        if not HEALTH_PING:
            logger.info("First health ping received")
            HEALTH_PING = True
        return connection.respond(http.HTTPStatus.OK, "Ok")

error message received:

2024-09-28 16:43:02,233 - INFO - First health ping received
ERROR:websockets.server:opening handshake failed
Traceback (most recent call last):
  File "/Users/trey.peacock/src/project/.venv/lib/python3.11/site-packages/websockets/asyncio/server.py", line 358, in conn_handler
    await connection.handshake(
  File "/Users/trey.peacock/src/project/.venv/lib/python3.11/site-packages/websockets/asyncio/server.py", line 209, in handshake
    raise self.protocol.handshake_exc
websockets.exceptions.InvalidStatus: server rejected WebSocket connection: HTTP 200

[aaugustin]

Yes, I noticed that as well last week, it's less than ideal. I'm going to file a new issue about this.