Add a handler for managing connections on the server #1762
Conversation
Motivation: Servers must manage connections created by clients. Part of this is gracefully closing connections (by sending GOAWAY frames and ratcheting down the last stream ID) in response to various conditions: the client sending too many pings, the connection being idle too long, the connection existing for longer than some configured limit, etc. A previous change added a state machine which handles much of this behaviour. This change adds a channel handler which builds on top of that state machine. Modifications: - Add a channel handler for managing connections on the server. Result: We have a handler in place which can manage connections on the server.
| } | ||
|
|
||
| func channelRead(context: ChannelHandlerContext, data: NIOAny) { | ||
| self.inReadLoop = true |
There was a problem hiding this comment.
Adding this comment here but it applies to more of this PR. Why do we have some state management inside a state machine but that some state managed as vars of this handler. It isn't clear to me where that separation goes and it feels to me like we should have a single state machine for this handler that handles all the state.
There was a problem hiding this comment.
Layering, mostly. Most of the state within the handler that isn't the state machine are triggers which prod the state machine. Storing timers etc., back in the state machine complicates the state machine more than it helps. As for the flushing etc, it's not related to connection management, it's its own separate set of state.
stefanadranca
left a comment
stefanadranca
left a comment
There was a problem hiding this comment.
Looks good to me!
Sources/GRPCHTTP2Core/Server/Connection/ServerConnectionManagementHandler.swift
Outdated
Show resolved
Hide resolved
gjcairo
left a comment
gjcairo
left a comment
There was a problem hiding this comment.
LGTM, just left a nit and a question.
Sources/GRPCHTTP2Core/Server/Connection/ServerConnectionManagementHandler.swift
Outdated
Show resolved
Hide resolved
| while self.flushPending { | ||
| self.flushPending = false | ||
| context.flush() |
There was a problem hiding this comment.
This is more of a NIO question but want to double check: context.flush() will start by calling self.flush() in this case, right? Which would in turn call maybeFlush(context:), which can reset flushPending to be true again.
I just want to make sure I understand why we're checking for flushPending in a while loop.
There was a problem hiding this comment.
No, context.flush() will call flush(context:) on the next channel handler.
We're calling it in a loop here in case a flush re-entrantly triggers another write which could set flushPending back to true.
Co-authored-by: Stefana-Ioana Dranca <66513820+stefanadranca@users.noreply.github.com> Co-authored-by: Gustavo Cairo <me@gustavocairo.com>
Motivation: Servers must manage connections created by clients. Part of this is gracefully closing connections (by sending GOAWAY frames and ratcheting down the last stream ID) in response to various conditions: the client sending too many pings, the connection being idle too long, the connection existing for longer than some configured limit, etc. A previous change added a state machine which handles much of this behaviour. This change adds a channel handler which builds on top of that state machine. Modifications: - Add a channel handler for managing connections on the server. Result: We have a handler in place which can manage connections on the server.
4 participants
Motivation:
Servers must manage connections created by clients. Part of this is gracefully closing connections (by sending GOAWAY frames and ratcheting down the last stream ID) in response to various conditions: the client sending too many pings, the connection being idle too long, the connection existing for longer than some configured limit, etc.
A previous change added a state machine which handles much of this behaviour. This change adds a channel handler which builds on top of that state machine.
Modifications:
Result:
We have a handler in place which can manage connections on the server.