move ssl_handshake handling from main thread to worker threads #750
Conversation
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
❌ 21 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
16 tasks
There was a problem hiding this comment.
Pull Request Overview
This PR shifts SSL handshake processing from the main thread to worker threads by introducing an explicit get() method in the thread pool and adding a do_handshake API to the built-in SSL adapter.
- Removed the
self.get = ...alias and implementedThreadPool.get()to perform the SSL handshake if needed. - Updated the built-in SSL adapter to defer handshake on wrap and provide a
do_handshakemethod with error handling and plaintext fallback. - Synchronized type stubs (
.pyi) with the newgetanddo_handshakesignatures.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cheroot/workers/threadpool.pyi | Removed old get attribute and added a typed def get(). |
| cheroot/workers/threadpool.py | Added get() logic to invoke SSL handshake; removed alias. |
| cheroot/ssl/builtin.pyi | Added do_handshake(conn) -> None stub. |
| cheroot/ssl/builtin.py | Switched off auto handshake in wrap() and implemented do_handshake with helpers. |
Comments suppressed due to low confidence (1)
cheroot/ssl/builtin.py:323
- New SSL handshake logic, including plaintext fallback and error branches, should be covered by unit or integration tests to ensure correct behavior under success, SSL errors, and plain-HTTP-over-HTTPS scenarios.
def do_handshake(self, conn):
| """ | ||
| conn = self._queue.get() | ||
| ssl_adapter = self.server.ssl_adapter |
There was a problem hiding this comment.
Accessing self.server.ssl_adapter directly can raise an AttributeError if the server has no ssl_adapter. Consider using getattr(self.server, 'ssl_adapter', None) to guard against missing attributes.
Suggested change
| ssl_adapter = self.server.ssl_adapter | |
| ssl_adapter = getattr(self.server, 'ssl_adapter', None) |
Copilot uses AI. Check for mistakes.
| try: | ||
| conn.socket.shutdown(socket.SHUT_RDWR) | ||
| except Exception: | ||
| # pass |
There was a problem hiding this comment.
The commented-out # pass line can be removed to clean up the code and avoid confusion about whether this exception block is intentional.
Suggested change
| # pass |
Copilot uses AI. Check for mistakes.
| Args: | ||
| conn (:py:class:`~cheroot.server.HTTPConnection`): HTTP connection | ||
| """ | ||
| ssl_handshake_must_be_done = ( |
There was a problem hiding this comment.
[nitpick] This do_handshake method is long and handles multiple concerns. Consider refactoring into smaller private helpers (e.g., handle_plain_http, log_handshake_error) for readability and easier maintenance.
Copilot uses AI. Check for mistakes.
3 tasks
|
@bmourgues I requested co-pilot to review, but don't take its comments too seriously (unless you double-check its suggestions by yourself): it's not good at understanding context. I haven't yet had time to make a proper review myself. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 kind of change does this PR introduce?
📋 What is the related issue number (starting with
#)Resolves #748
Resolves #709
❓ What is the current behavior? (You can also link to an open issue here)
❓ What is the new behavior (if this is a feature change)?
📋 Other information:
📋 Contribution checklist:
(If you're a first-timer, check out
this guide on making great pull requests)
the changes have been approved
and description in grammatically correct, complete sentences
This change is