Skip to content

Handle rejected handshakes cleanly in the threading server#1726

Open
apoorvdarshan wants to merge 1 commit into
python-websockets:mainfrom
apoorvdarshan:fix-sync-server-rejected-handshake
Open

Handle rejected handshakes cleanly in the threading server#1726
apoorvdarshan wants to merge 1 commit into
python-websockets:mainfrom
apoorvdarshan:fix-sync-server-rejected-handshake

Conversation

@apoorvdarshan

@apoorvdarshan apoorvdarshan commented Jul 9, 2026

Copy link
Copy Markdown

Summary

The threading server didn't handle a rejected opening handshake explicitly. When process_request or process_response returned an HTTP response, handshake() returned normally while the protocol state wasn't OPEN. The following assertion failed silently—or disappeared under python -O, allowing the handler to run on a closed connection and log "connection handler failed".

Fix

Replace the assertion with an explicit rejection path that closes the socket, joins the receive thread, and returns, mirroring the asyncio implementation.

Tests / Verification

  • Guarded test_process_request_returns_response with assertNoLogs("websockets", logging.ERROR).
  • Matched the asyncio tests by verifying the exact "opening handshake failed" error and exception for process_request and process_response failures.
  • Official coverage environment: 2,258 tests passed, 10 skipped, 100% coverage.
  • Ruff formatting and lint checks pass.
  • Strict mypy checks pass.
  • python -O -m unittest tests.sync.test_server: 50 tests passed.

Refs #1513.

Disclosure: prepared with AI assistance; manually reviewed and verified locally.

@aaugustin aaugustin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change the commit message to something shorter, e.g.:

Don't log an error when process_request returns a response.

Refs #1513.

We don't need a novella :-)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't add changelog entries for small fixes like this one (e.g. cdeb882, which fixed the same problem in the asyncio implementation, didn't include one).

Comment thread src/websockets/sync/server.py Outdated
assert connection.protocol.state is OPEN
if connection.protocol.state is not OPEN:
# process_request or process_response rejected the handshake.
# The connection is already closing or closed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this line. We're in a branch "state is not OPEN"; it's clear that it's closing or closed.

Comment thread tests/sync/test_server.py
str(raised.exception),
"server rejected WebSocket connection: HTTP 403",
)
with self.assertNoLogs("websockets", logging.ERROR):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at cdeb882, you'll see that it added assertNoLogs assertions to a bunch of tests in the asyncio implementation.

Likely, the same tests should have assertNoLogs in the threading implementation. Can you check and add where appropriate please?

@aaugustin

Copy link
Copy Markdown
Member

The failure in GitHub Actions looks spurious :-/

@aaugustin

Copy link
Copy Markdown
Member

Thank you, this looks valid. I'd like a few changes; you're welcome to update the PR if you'd like; or I can make them myself and take it from there.

@apoorvdarshan apoorvdarshan force-pushed the fix-sync-server-rejected-handshake branch from 78ccb0d to 3eba136 Compare July 10, 2026 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants