gh-109051: fix start_tls() on paused-for-writing transport #109603
Conversation
sorcio
requested review from
1st1,
asvetlov,
gvanrossum,
kumaraditya303 and
willingc
as code owners
|
CI failure is unrelated to this, already reported: #109582 |
| # as possible. | ||
| _, high_water = transport.get_write_buffer_limits() | ||
| buffer_size = transport.get_write_buffer_size() | ||
| writing_paused = buffer_size > high_water |
There was a problem hiding this comment.
Would it safe to only use this code path? (remove fast-path for _FlowControlMixin)
There was a problem hiding this comment.
Good question, and I'm not sure. I saw the pattern used by _SendfileFallbackProtocol which checks for the mixin. It's the only other outstanding use case for set_protocol() in the stdlib.
Pedantically speaking, the documentation doesn't make an unambiguous claim that pause_writing() is called instantly when the buffer is filled above the high water mark, or that a background thread (or a proactor callback? I don't truly understand that part) doesn't drain the buffer in a way that is accidentally observable. But I don't think any transport in the stdlib is susceptible to this.
A notable case happens if the high water mark is increased while paused, and before start_tls. In the current implementation, the limit returned by get_write_buffer_limits() would be the increased one, but resume_writing() would not be called yet until the next opportunity to drain.
There was a problem hiding this comment.
In short, the answer is no, I don't think it would be safe. But I'd defer to an asyncio expert.
There was a problem hiding this comment.
In case of doubt, keep the specific code path for _FlowControlMixin.
|
@kumaraditya303 @willingc @gvanrossum: How do you feel about reviewing this asyncio fix for SSL? Do you know someone who can review such change? |
Added a
writing_pausedargument to SSLProtocol() so it can wrap the existing transport+protocol in any state. Also added a test to specifically trigger the failing case on any platform.