Add send_eof to SSLStream and get rid of HalfCloseableStream

[njsmith]

We have an awkward thing in our stream abstraction: there's the abstract Stream interface, and then there's the abstract HalfCloseableStream interface, which is just Stream with an added send_eof method.

In practice, every single Stream you'd ever meet in real life – TCP connections, StapledStream, SSH tunnels, HTTP/2 streams, etc. etc. etc. – is a HalfCloseableStream. Except... SSLStream, because for some reason TLS doesn't support half-close. And unfortunately TLS is super important in practice, so even though in some abstract sense it's TLS that's broken and fails to implement a proper byte stream, we instead broke our Stream interface to accomodate TLS.

BUT! It turns out that TLS 1.3 fixed this! Quoting RFC 8446 §6.1:

Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert. This does not have any effect on its read side of the connection. Note that this is a change from versions of TLS prior to TLS 1.3 in which implementations were required to react to a "close_notify" by discarding pending writes and sending an immediate "close_notify" alert of their own. That previous requirement could cause truncation in the read side. Both parties need not wait to receive a "close_notify" alert before closing their read side of the connection, though doing so would introduce the possibility of truncation.

Also @kroeckx says:

OpenSSL has supported that [= half-close] for a very long time, even when the older TLS standards didn't say that was supported.

So TLS 1.3 now has the same semantics as every other common byte-stream transport, and apparently in some cases we can get that with earlier versions too, if we cross our fingers and the moon is full.

I was already on the fence about whether we should have two separate types for Stream and HalfCloseableStream, versus merging them into a single type and just saying that send_eof might fail in some cases. This clearly tips things over, given that it means SSLStream is going to want to implement send_eof.

We'll still have to document clearly that (a) send_eof may not work in all cases, and (b) even if it works, there's lots of software out there that doesn't really understand half-close. But at least we can drop HalfCloseableStream. I never liked that thing.

[oremanj]

While we're adding optional half-close methods to the Stream interface, maybe we want to add the other one? ie "stop reading", analogous to shutdown(SHUT_RD) for a socket or the "end-of-write" channel request on an OpenSSH channel. Not all protocols support it (many fewer than half-close on the write side, I suspect) but it's a nice way of propagating backpressure in those that do. I've been calling this operation shutdown_read() locally.

[njsmith]

Does openssh support it? TCP doesn't – if you use SHUT_RD, that doesn't send any message to the remote peer. All it does is set a local flag saying "if we do receive any more data from the remote peer, then immediately send a RST to blow up the whole connection", which is not very useful.

Also, how are you imagining the sending code would find out that backpressure was being propagated?

[oremanj]

OpenSSH does definitely support this - it's how ssh remotehost yes | head is able to ultimately cause the remote yes process to receive SIGPIPE. It's an OpenSSH extension though, not in the main SSH standard. I don't know how other implementations do with it. example:

$ cat t.py
import sys
import itertools
for idx in itertools.count():
  try:
    print("this is line {}".format(idx))
  except BrokenPipeError:
    print("got to line {}".format(idx), file=sys.stderr)
    print("type something:", file=sys.stderr)
    line = input()
    print("you said {!r}".format(line), file=sys.stderr)
    break

$ python3.6 t.py | head
this is line 0
this is line 1
this is line 2
this is line 3
this is line 4
this is line 5
this is line 6
this is line 7
this is line 8
this is line 9
got to line 970
type something:
this is a test
you said 'this is a test'

$ ssh localhost python3.6 t.py | head
Warning: Permanently added 'localhost' (ECDSA) to the list of known hosts.
this is line 0
this is line 1
this is line 2
this is line 3
this is line 4
this is line 5
this is line 6
this is line 7
this is line 8
this is line 9
got to line 1427
type something:
this is a test
you said 'this is a test'

Other uses: the current draft of QUIC (which supposedly will be the basis for HTTP/3) has a STOP_SENDING request that maps to read-side-only half-close very nicely.

Also, how are you imagining the sending code would find out that backpressure was being propagated?

In terms of the interface: if the receiver shuts down their receive side, the sender should probably get BrokenResourceError when they try to send. In terms of the implementation, this requires some way for the receiver to tell the sender they don't want more data. It's definitely a bit of a niche feature, and we can document it as such.

If nothing else, StapledStream can support it pretty trivially!

[njsmith]

Given that none of TCP, TLS, standard SSH, or HTTP/2 actually support shutdown_read, it seems like something to leave for individual classes instead of standardizing as part of Stream... StapledStream already has an API for this (await stream.send_stream.aclose(), await stream.receive_stream.aclose()), and didn't we decide earlier that the best way to model an SSH stream was as multiple unidirectional stream objects? That would make StapledStream a pretty natural way to expose it...

[oremanj]

Especially after reading your comments on #895, leaving out shutdown_read seems like the obviously correct choice -- thanks for the explanation!

Back on the write-side close front, I don't know what semantics you had in mind for the generalization of send_eof(), but I'm currently imagining a method in the Stream ABC with a slightly more general name like shutdown(), which would have the semantics of send_eof() if the underlying protocol supports those semantics, or else at minimum would guarantee not to impose any unnecessary restrictions on continuing to read data (vs aclose() which as a matter of interface contract causes receive_some() to immediately start raising ClosedResourceError). That would let a generic Stream-based class implement graceful close as "stop sending, call shutdown(), continue receiving until receive_some() returns EOF, call aclose()". And if the underlying protocol drops data under those circumstances, well, that's the underlying protocol's fault, we did our best. If combined with appropriate documentation on the caveats, with examples, I think this would be less error-prone than a send_eof() that on non-half-closey protocols would do nothing or raise NotImplementedError.

Another option (somewhat more provocative) would be to state that if the underlying protocol doesn't support reliable half-close, i.e., if the sequence of operations described above could lose data from the remote peer in some cases, then it should be implemented so that receiving EOF after calling shutdown() always raises BrokenResourceError (or some other exception, "IndeterminateCloseError"?) out of receive_some(). The theory behind this is that if you don't care about all of the data getting through, you probably won't use shutdown(); and if you do care about all of the data getting through even on non-half-closey protocols, you should have an application-layer close message, and can/should ignore transport errors that come after that message. I'm not sure if I actually recommend this - it seems like a predominant effect would be to create confusion when protocols that worked fine on bare TCP start raising errors at close time when tunneled over TLS - but I mention it because it's one way we could take a stand for "better to fail fast consistently than to screw up quietly in rare circumstances", which seems in line with some of our values around here. :-)

[njsmith]

I don't know what semantics you had in mind for the generalization of send_eof()

I was just imagining that we'd add it to Stream, with a default definition of raise NotImplementedError.

let a generic Stream-based class implement graceful close as "stop sending, call shutdown(), continue receiving until receive_some() returns EOF, call aclose()". And if the underlying protocol drops data under those circumstances, well, that's the underlying protocol's fault, we did our best

I'm more worried about the overlying protocol :-). The way I think about it, send_eof is a message that can be transported over the stream, that a given protocol might or might not assign some meaning to. (That's why it has send in the name.) OTOH aclose is about resource management – its contract is that it will tear down the stream and free the associated resources, and that might incidentally have effects that are visible at the next level up (e.g., the other end receiving an EOF).

So for protocols that want to assign particular meaning to sending an EOF, then the NotImplementedError approach is more helpful – either it does what you expect, or it does nothing, and then you can figure out how to cope with that.

And then there are protocols where send_eof doesn't mean "close this connection ASAP please". Technically HTTP/1.1 falls into this category... it's supposedly legal for an HTTP client to send a request and then immediately shutdown(SHUT_WR), and read the arbitrarily-large body. I suspect most servers don't respond well to this, but if someone's using asks and decides they don't want to read the body, so they call aclose, and they happen to be talking to a server that does handle this correctly... we don't want that aclose to go reading the whole body before closing the socket. Or here's a simpler example: using a SocketStream as a ReceiveStream. If the other side isn't calling recv, then they won't even notice our shutdown(SHUT_WR), so aclose would just hang indefinitely.

And then there's a practical problem: A lot of experience socket programmers have no idea that shutdown exists, or if they've heard of it they're confused about what it means. I think if we make SocketStream.aclose do anything except call self.socket.close() then we'll alienate a bunch of users.

[oremanj]

Oh, yeah, I absolutely agree that SocketStream.aclose should tear down the connection in its entirety without waiting on the remote peer. I think the only place where my thoughts might differ from the picture you've painted above is in the handling of protocols that, uh, sorta support half-close.

Taking a step back, it's definitely true that the EOF is a message whose meaning is ultimately determined by the protocol, but I think there's also a broadly recognized strategy for tearing down a full-duplex connection in such a way that both peers agree on what data was sent, which shows up in a lot of protocols. Something like:

• If you want to close the connection, and you haven't heard that the other side wants to close the connection:
  • Send any data you had enqueued.
  • Send EOF.
  • Keep receiving until you receive EOF.
  • Close the connection.
• If you receive EOF before you want to close the connection:
  • Send any data you had enqueued / data that constitutes responses to requests made by the other side before their EOF.
  • Close the connection.

If run over a transport that fully supports half-close, this works great; both sides send and receive EOF, so both can agree on what data was sent: "everything before the EOF marker". What about transports that claim to not support half-close, but still have shutdown procedures that look sort of like the graceful close procedure I described above?

• Example: TLS pre-1.3. We can turn send_eof into sending a close_notify, and we can keep reading after that until we get a close_notify in return. If we're talking to OpenSSL, this will probably even work correctly. But apparently the spec says that our peer in this case is supposed to discard pending writes in order to send its responding close_notify as quickly as possible. So, we can faithfully follow the above procedure, and still not receive all the data our peer intended for us to receive before EOF.
• Example: Websockets. Pretty much the same story - my read of the spec is that receiving a CLOSE frame requires you to send one back in response, even if you had some data enqueued.

Both of these have some interesting properties:

• It's easy for an endpoint to drain their queue of outgoing data-to-send after receiving the initial close message and before sending theirs in response. It's hard to argue that that's even invalid - after all, who's to say that that additional data wasn't already in the network by the time they got our close message? So, there's a reasonable chance that our peer might be more half-close-supporting than the spec says they have to be. (It seems to me like this is exactly what was going on for OpenSSL pre-TLS-1.3.)
• Even for peers that drop data-to-be-sent when they receive a close request: if we act like we support half-close for these protocols, we can receive more data from our peer than if we don't. If we send the close frame and keep receiving data until we receive a close frame in response, we're able to drain the data that was in the local socket receive buffer when we sent our close frame and the data that our peer sent during the time our close frame was traveling from us to them. Probably also the data that was in the remote socket send buffer when our peer received our close frame. Maybe not the data that was in our peer's application-level buffers. But if we just aclose(), we won't get any of that data. Maybe this makes the difference between receiving the end of the response to our request versus not receiving it.

If you try to do the graceful-close dance above, and send_eof() raises NotImplementedError, you have to call aclose(). But if send_eof() sends whatever the local "I'm going away now" message is, even if it might make the peer drop some of the data it was planning to send, you have a chance to keep receiving data that you otherwise would've had to drop. That seems to me like an unfortunate capability to throw away.

Bonus question: what should we do for SSLStream.send_eof()? Should it raise NotImplementedError unless we're speaking TLS 1.3+ or our peer is using OpenSSL? Or should it send close_notify, keep the receive side open, and hope for the best? If we're OK hoping for the best in SSLStream, maybe we should be OK with it everywhere?

(Any protocol implemented on top of TCP can support this "sorta half-close", even if it doesn't exchange protocol-level close messages, because you can send a FIN and keep receiving until you get one in response. I'm tempted to say that any application network protocol at all kind of has to support it, because of the data that might be in the network at the time you close the connection, even if some libraries-that-implement-protocols might not expose the ability. But there might be a case I'm missing.)

[oremanj]

Another thought: If you have a protocol that does assign semantics to the EOF marker of the underlying transport, it's a type error to use it on a transport that doesn't support sending an EOF marker. It's better for this error to be obvious statically than for everything to seem to work fine until the protocol wants to send the EOF marker. To me this argues that either we should require all transports (Stream implementations) to support sending an EOF marker, even if the outcome might be imperfect somehow, or we should keep distinguishing between Stream and HalfCloseableStream.

[njsmith]

I think there's also a broadly recognized strategy for tearing down a full-duplex connection in such a way that both peers agree on what data was sent, which shows up in a lot of protocols

I agree that there's a common pattern for accomplishing this, but I would have said that the common pattern is to never send EOF, and instead do all the shutdown handling by sending bytes back and forth. E.g., websocket and HTTP/2 are both in this class. Arguably HTTP/1.1 as well. And of course any protocol that wants to work over TLS can't assume the existence of send_eof. Do you have any examples of protocols/implementations that use the strategy you described?

If you try to do the graceful-close dance above, and send_eof() raises NotImplementedError, you have to call aclose(). But if send_eof() sends whatever the local "I'm going away now" message is, even if it might make the peer drop some of the data it was planning to send, you have a chance to keep receiving data that you otherwise would've had to drop. That seems to me like an unfortunate capability to throw away.

Bonus question: what should we do for SSLStream.send_eof()?

That's not really a bonus question. SSLStream is the only stream implementation I know of where send_eof might ever raise NotImplementedError, so really the whole discussion is about what SSLStream should do and how it fits in :-). Speaking of which:

It's better for this error to be obvious statically than for everything to seem to work fine until the protocol wants to send the EOF marker.

In a perfect world, yes, but we don't know until after the TLS handshake what version of TLS we're using, and whether it supports send_eof.

I guess a two important principles for me also are: (1) there's a general consensus around "how a stream works", that's shared across at least the designers of TCP, SSH2, HTTP/2, and modern TLS... it seems very natural to me to use this as a guide for trio's Stream too. (2) Methods with vague and undefined semantics are generally a bad idea?

[oremanj]

I agree that there's a common pattern for accomplishing this, but I would have said that the common pattern is to never send EOF, and instead do all the shutdown handling by sending bytes back and forth. E.g., websocket and HTTP/2 are both in this class. Arguably HTTP/1.1 as well. And of course any protocol that wants to work over TLS can't assume the existence of send_eof. Do you have any examples of protocols/implementations that use the strategy you described?

Touché. I suspect I'm guilty here of doing too much thinking about how I would design an ad-hoc protocol for a niche need, and too little research into what widely used protocols do. And half-close is so obviously the way a byte stream "should work" that I think I want it to work in more places than maybe it actually does. :-)

I'm on board with having send_eof() as a method of Stream that is always present but conditionally implemented. Sorry for needing so much back-and-forth to get there.

[njsmith]

I'm on board with having send_eof() as a method of Stream that is always present but conditionally implemented. Sorry for needing so much back-and-forth to get there.

Thank you for needing so much back-and-forth, it forces us to figure out the details and generate a record of the rationale :-)