fix eof_received #661
fix eof_received #661
Conversation
|
@fantix this is your area of expertise :) |
WalkthroughThis PR improves SSL/TLS shutdown handling in uvloop by refining the SSLProtocol state machine and introducing a new test for close_notify alert processing. The changes clarify state transitions during connection closure, add a dedicated method for read-and-shutdown logic, and enhance test coverage for SSL close_notify scenarios in asynchronous contexts. These updates aim to ensure correct resource management and protocol compliance during SSL shutdown events. Changes
Sequence DiagramThis diagram shows the interactions between components: sequenceDiagram
title Connection Shutdown and Flush Operations
participant Client
participant Connection
Note over Connection: The PR adds a new method: _do_read_flush()
Client->>Connection: initiate shutdown
activate Connection
Connection->>Connection: _start_shutdown(context)
alt normal operation flow
Connection->>Connection: _do_read_into_void(context)
Connection->>Connection: _do_flush(context)
Connection->>Connection: _do_shutdown(context)
else with new read flush capability
Connection->>Connection: _do_read_into_void(context)
Connection->>Connection: _do_read_flush()
Note right of Connection: New method added in PR
Connection->>Connection: _do_flush(context)
Connection->>Connection: _do_shutdown(context)
end
Connection-->>Client: shutdown complete
Connection->>Connection: _on_shutdown_complete(shutdown_exc)
loop until shutdown completes or times out
Connection->>Connection: _check_shutdown_timeout()
end
deactivate Connection
Note for WindsurfPlease change the default marketplace provider to the following in the windsurf settings:Marketplace Extension Gallery Service URL: https://marketplace.visualstudio.com/_apis/public/gallery Marketplace Gallery Item URL: https://marketplace.visualstudio.com/items Entelligence.ai can learn from your feedback. Simply add 👍 / 👎 emojis to teach it your preferences. More shortcuts belowEmoji Descriptions:
Interact with the Bot:
Also you can trigger various commands with the bot by doing The current supported commands are
More commands to be added soon. |
| cdef _do_read_flush(self): | ||
| self._do_read() | ||
| self._set_state(SHUTDOWN) | ||
| self._on_shutdown_complete(None) |
There was a problem hiding this comment.
Correctness: The _do_read_flush method is called in the FLUSHING state but doesn't handle the case when app reading is paused, which could lead to data loss when the client pauses reading.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| cdef _do_read_flush(self): | |
| self._do_read() | |
| self._set_state(SHUTDOWN) | |
| self._on_shutdown_complete(None) | |
| cdef _do_read_flush(self): | |
| if not self._app_reading_paused: | |
| self._do_read() | |
| self._set_state(SHUTDOWN) | |
| self._on_shutdown_complete(None) |
I think it's not right to shutdown when eof received. It's better to wait for app resume when app reading paused. I wrote a test case(test_close_notify.py) for this situation. The server first sends 1024 * 50 bytes to client, then sends 7 bytes. The client reads the first payload(1024 * 50) then pauses reading and handles the payload in 3 seconds. After that the client resumes reading. So the client should totally receive 1024 * 50 + 7 bytes, not just 1024 * 50 bytes.
Even when there are no app reading pause, reading from buffer is needed before shutdown, cause sometimes ssl eof is in the same package of tcp fin. So when connection is closing, reading buffer is still needed to parse the EOF alert message in this package.