Add CloseableConnection capability - #94
Conversation
| /// - On HTTP/1.1, the response carries `Connection: close` and the | ||
| /// channel is closed once the response has been written. |
There was a problem hiding this comment.
We should document that signalConnectionClose() must be called before sending the response for this to hold true.
There was a problem hiding this comment.
Unless I'm misunderstanding what you're saying, this isn't true. We can call signalConnectionClose() mid-response and that's okay, we will close once response has been written.
There was a problem hiding this comment.
I was talking about initiating connection close after the response header has been flushed.
I think this doc should be updated with these two things:
- The
Connection: closeheader cannot be written if the response head was already flushed beforesignalConnectionClose()being called. - In this case though, the channel will be closed after the end part is written (following from your change in commit d0688c1).
08afd1b to
2d04886
Compare
| // Drain the body so `invokeHandler` recovers the iterator and the | ||
| // request loop stays alive — otherwise the loop exits when the | ||
| // handler returns, and the dispatcher closes the channel anyway. | ||
| var body = UniqueArray<UInt8>() | ||
| body.reserveCapacity(1024) | ||
| _ = try await reader.collect(into: &body) |
There was a problem hiding this comment.
This test seems very similar to the one above. These lines are the only difference I see compared to the test above. But I don't think this should matter because the client in this test case only sends a head and end part.
There was a problem hiding this comment.
It does matter because of the way we handle the ReaderState internally when we finish reading vs when we don't (see NIOHTTPServer/invokeHandler). Also, another difference is we're not setting any timeouts in this test: we want to make sure the connection is being closed and not hang indefinitely/be closed by other timeout mechanisms instead of close being signalled.
aryan-25
left a comment
aryan-25
left a comment
There was a problem hiding this comment.
Thank you! Just one small comment.
| /// A request-context capability that lets a request handler signal that the | ||
| /// connection should close after the current response. | ||
| /// | ||
| /// Servers whose request context conforms to this capability allow handlers | ||
| /// to indicate that the underlying connection should be closed once the | ||
| /// in-flight response has been sent. Implementations make the signal | ||
| /// effective on a best-effort basis (HTTP/1.1 typically appends | ||
| /// `Connection: close` and closes the channel; HTTP/2 typically sends | ||
| /// `GOAWAY` and lets in-flight streams complete normally). | ||
| public protocol CloseableConnection: RequestContext { |
There was a problem hiding this comment.
I find the semantics a bit weird that this only closes after the response is send. Why doesn't it close right away?
There was a problem hiding this comment.
For H1, closing after the response is sent allows users to write back a response that will contain the Connection:close header, because we'll set it when signalling close.
For H2, I think it's weird to just abruptly close the connection because there can be other streams open. We currently send GOAWAY but I suppose we could send RST_STREAMs instead?
I think just abruptly closing the connection feels too harsh, but perhaps that's the desired behaviour.
cc @ehaydenr
There was a problem hiding this comment.
I think you want to be able to do both. If you're shutting down, you may first want to send a graceful GOAWAY to avoid disrupting in flights requests. After some grace period, you may follow up with a more abrupt GOAWAY followed by connection close which would break streams. Alternatively, you may want to immediately close things abruptly if the peer is behaving badly.
There was a problem hiding this comment.
You think it would make sense to be able to initiate both from here, or would you have separate capabilities?
I've not given this much thought but one thing that kinda bothers me is that force closing doesn't stop you from still attempting to read or write within this handler, and that will cause errors. Basically the only thing you can do after a force close is to return. The only way I can think of doing this is by having the force close consume the reader and response sender/writer, but I don't think that's particularly nice either.
There was a problem hiding this comment.
I think if you force close and then do a read or write, it should probably result in an error and that's OK
There was a problem hiding this comment.
@ehaydenr do you wanna be able to do both the graceful shutdown and the force closure from a stream? I am actually wondering if we are doing this the wrong way around. In my opinion, having this as a stream capability is really weird and it would be better served as API on Connection. So what I am suggesting is that Connection gains methods for triggerGracefulShutdown, close and maybe a combined one that triggers graceful shutdown and after a duration closes. You can then inject an MPSC channel into your stream handlers that will do this on the connection. What do you think about this?
There was a problem hiding this comment.
I want to be able to do it from a request hander. Whether it's exposed as a RequestContext capability or I need to plumb it through to where I instantiate my request handler, I don't think it matters. Exposing as a capability seems like the natural way to accomplish this. What about it do you find weird?
There was a problem hiding this comment.
I continue to think it is weird to expose connection lifecycle management methods on request context. I feel like we will end up with a god request that will allow you to do everything with the connection. I much prefer clear separation of concerns between the request handler and the mostly read-only context of the connect and a separate connection lifecycle API. If users want to tie the two together they can setup a communication channel between those.
4 participants
This is a follow-up of the connection lifecycle PR: #92
It adds support for signalling that the current connection should close after the in-flight response via a new
CloseableConnectionHTTPServerCapability.NIOHTTPServer.RequestContextconforms to it and exposes this capability viarequestContext.signalConnectionClose().