Make send_eof a standard part of the Stream interface#1181
Make send_eof a standard part of the Stream interface#1181njsmith wants to merge 7 commits intopython-trio:mainfrom
Conversation
The only reason HalfCloseableStream existed as a separate interface was because SSLStream couldn't support send_eof. But TLS 1.3 made it possible to support send_eof! So now the static split doesn't make sense. Instead, we move send_eof into the Stream interface (though it can still fail at runtime with NotImplementedError), and get rid of HalfCloseableStream. (Fixes: python-triogh-823.) This also includes a fix for StapledStream.send_eof, that was revealed by the new enhanced check_two_way_stream. (Fixes: python-triogh-1180).
|
This looks great overall. One yapf nit to fix, and one backwards compatibility concern: adding
I'm also OK leaving this incompatibility in, but it should probably be called out more explicitly in the newsfragments in that case. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Still need to fix up some coverage. |
|
OK, I think everything's been addressed now. |
| ): | ||
| await trio.hazmat.checkpoint() | ||
|
|
||
| def do_read(): |
There was a problem hiding this comment.
one thought here is that maybe it would be better to move this function out into it's own method? i don't have any really strong opinion on whether or not that makes that code more readable. but i just wanted to bring it up to see what others think. i definitely think that this is cleaner than the previous approach.
|
|
||
| async def wait_send_all_might_not_block(self): | ||
| if self.socket.did_shutdown_SHUT_WR: | ||
| raise trio.ClosedResourceError("can't send data after sending EOF") |
There was a problem hiding this comment.
i'm not exactly sure where these changes fit in to the issues this MR is fixing. perhaps there is something that can be added to the newsfragment? it seems related related to what you were fixing in #1180 but it's in a different class.
There was a problem hiding this comment.
I don't remember exactly anymore, but I think the sequence was (1) I reworked the send_eof tests in trio.testing.check_*, (2) SocketStream started failing the more rigorous tests, (3) I fixed SocketStream to pass the tests again. I'll add a newsfragment...
| return await self.send_stream.send_eof() | ||
| else: | ||
| with self._send_conflict_detector: | ||
| if hasattr(self.send_stream, "send_eof"): |
There was a problem hiding this comment.
now that all classes inheriting from Stream are required to implement send_eof, why do we need to check that the sending stream has the send_eof method here? i think i am probably misunderstanding a subtle point here. maybe this is for backwards compatibility or maybe there are other types of streams that i am unaware of, which can be "stapled" together.
There was a problem hiding this comment.
All Streams have to implement send_eof, but unidirectional SendStreams don't have to, and in this method, all we know about self.send_stream is that it's a SendStream, not necessarily a Stream.
I'm not actually sure if this opportunistic attempt to call send_eof accomplishes anything useful... it would be simpler, and maybe better, to unconditionally call aclose. But I decided this PR was already doing enough, so I'd leave worrying about that to another time...
|
Sorry for dropping this. I took another look just now and I'm happy with it, except that the version numbers of the deprecations need to be increased from 0.13.0 to 0.15.0. |
4 participants
The only reason HalfCloseableStream existed as a separate interface
was because SSLStream couldn't support send_eof. But TLS 1.3 made it
possible to support send_eof! So now the static split doesn't make
sense. Instead, we move send_eof into the Stream interface (though it
can still fail at runtime with NotImplementedError), and get rid of
HalfCloseableStream. (Fixes: gh-823.)
This also includes a fix for StapledStream.send_eof, that was revealed
by the new enhanced check_two_way_stream. (Fixes: gh-1180).