Channel quiescing support #399

weissi · 2018-05-09T18:22:10Z

Motivation:

In certain cases it's useful to quiesce a channel instead of just
closing them immediately for example when receiving a signal.
This lays the groundwork by introducing the
ChannelShouldQuiesceUserEvent user event that when received can be
interpreted by a ChannelHandler in a protocol & application specific
way. Some protocols support tear down and that would be a good place to
initiate the tear down.

Modifications:

introduce ChannelShouldQuiesceUserEvent
handle ChannelShouldQuiesceUserEvent in the AcceptHandler with
closing the server socket
handle ChannelShouldQuiesceUserEvent in the
HTTPServerPipelineHandler by only handling a already in-flight
request and then no longer accepting input

Result:

handlers can now support quiescing

Lukasa · 2018-05-14T15:17:29Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

                // We got a response while still receiving a request, which we have to
                // wait for.
                self = .requestEndPending
-            case .requestEndPending:
+            case .requestEndPending, .idle:
                preconditionFailure("Received second response")


This text would probably need updating.

Lukasa · 2018-05-14T15:17:36Z

Sources/NIO/CircularBuffer.swift

@@ -204,6 +204,13 @@ public struct CircularBuffer<E>: CustomStringConvertible, AppendableCollection {
        return nextIndex
    }

+    /// Removes all members from the


Truncated comment.

Lukasa · 2018-05-14T15:17:41Z

Sources/NIO/Channel.swift

@@ -362,3 +362,8 @@ public enum ChannelEvent: Equatable {
    /// Output portion of the `Channel` was closed.
    case outputClosed
 }
+
+public struct ChannelShouldQuiesceUserEvent {


Needs docs.

Lukasa · 2018-05-14T15:18:26Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

+            if self.lifecycleState == .noLongerAcceptingEvents && self.state == .idle {
+                ctx.close(promise: nil)
+            }
+        default:


No need for default here, we know what the third enum case is.

Lukasa · 2018-05-14T15:19:19Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

        ctx.fireChannelRead(data)
    }

+    private func deliverOneError(ctx: ChannelHandlerContext, error: Error) {
+        if (self.state == .idle || self.state == .requestEndPending) && error is HTTPParserError {
+            self.state = .responseEndPending


Would be nice to document this logic.

Lukasa · 2018-05-14T15:21:00Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

-        if case .end = self.unwrapOutboundIn(data) {
+
+        switch self.unwrapOutboundIn(data) {
+        case .head(var head) where self.lifecycleState != .acceptingEvents:


Is this over-broad? Should it just be when lifecycleState is quiescing?

@Lukasa I think != .acceptingEvents which is the same as saying .quiescing || .noLongerAcceptingEvents is correct. The reason is that we should add a connection: close header whenever we're quiescing. The .quescing case however means that we're continuing to accept request parts (like .body and .end). .noLongerAcceptingEvents means that we're quiescing and we have already seen the request .end.

I renamed the cases to .quiescingWaitingForRequestEnd and .quiescingLastRequestEndReceived which should make this clearer

Lukasa · 2018-05-14T15:22:33Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

+                ctx.write(data).then {
+                    ctx.close()
+                }.cascade(promise: promise ?? ctx.channel.eventLoop.newPromise())
+            default:


No need for this default.

Lukasa · 2018-05-14T15:22:39Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

+            default:
+                ctx.write(data, promise: promise)
+            }
+        default:


No need for this default.

Lukasa · 2018-05-15T09:33:08Z

Sources/NIOHTTP1/HTTPServerPipelineHandler.swift

+            switch self {
+            case .idle:
+                self = .requestAndResponseEndPending
+            default:


Might be better to explicitly enumerate all the states here?

Or use an if case .idle = self, as it seems just one case is evaluated.

@BasThomas we want to crash in the other cases; @Lukasa ok

Lukasa · 2018-05-15T15:14:18Z

Tests/NIOHTTP1Tests/HTTPServerPipelineHandlerTest.swift

+
+
+
+


Bunch of whitespace down here.

must have fallen asleep on my space key...

normanmaurer · 2018-05-15T19:07:07Z

Sources/NIO/Channel.swift

+/// The action(s) that should be taken after receiving this event are both application and protocol dependent. If the
+/// protocol supports a notion of requests and responses, it might make sense to stop accepting new requests but finish
+/// processing the request currently in flight.
+public struct ChannelShouldQuiesceUserEvent {


nit: consider using an enum ?

@normanmaurer we need to be able to instanciate that one. So I could do an enum but what would the cases be? I could only think of one case "please quiesce the channel" and I thought that's easier expressed by just using a type. But happy to change. We can't use ChannelEvent because it's an enum and extending it would be a major version break

normanmaurer · 2018-05-15T19:07:31Z

Sources/NIO/Channel.swift

+/// The action(s) that should be taken after receiving this event are both application and protocol dependent. If the
+/// protocol supports a notion of requests and responses, it might make sense to stop accepting new requests but finish
+/// processing the request currently in flight.
+public struct ChannelShouldQuiesceUserEvent {


Also do we just want to call it: ChannelShouldQuiesceEvent ?

I followed TLSUserEvent but there's also ChannelEvent so there's prior art for both. I don't mind which one. @Lukasa ?

I think Event is better, I think the TLSUserEvent name wasn't great.

normanmaurer · 2018-05-15T19:08:57Z

Sources/NIO/CircularBuffer.swift

@@ -204,6 +204,13 @@ public struct CircularBuffer<E>: CustomStringConvertible, AppendableCollection {
        return nextIndex
    }

+    /// Removes all members from the circular buffer whist keeping the capacity.
+    public mutating func removeAll() {


In swift in other places we have removeAll(keepingCapacity:). Maybe we should use the same method naming and behaviour ?

https://developer.apple.com/documentation/swift/array/1641473-removeall

will do tomo

normanmaurer · 2018-05-15T19:11:10Z

Sources/NIOHTTP1/HTTPTypes.swift

+extension HTTPResponseHead {
+    /// Whether this HTTP response is a keep-alive request: that is, whether the
+    /// connection should remain open after the request is complete.
+    public var isKeepAlive: Bool {


I think we should implement it kind of the same way as here:

https://github.com/apple/swift-nio/pull/402/files#diff-9361bcb5608a311475f3a33eeadc3a3eR300

@normanmaurer ok, will do that tomorrow. Also the 'wrong' impl first?

@weissi let me just merge my pr and then you can reuse the headers.isKeepAlive

weissi · 2018-05-16T14:07:23Z

Tests/NIOTests/IntegerTypesTest.swift

+//
+//===----------------------------------------------------------------------===//
+
+import XCTest


note to @Lukasa / @normanmaurer : for a second i thought nextPowerOf2 had a bug but couldn't find the tests so added some

Lukasa · 2018-05-16T16:14:03Z

Sources/NIO/Channel.swift

+/// processing the request currently in flight.
+public struct ChannelShouldQuiesceEvent {
+    public init() {
+    }


This init is still here and probably not needed.

@Lukasa no, we need a public init and the default one is an internal init

Ah sorry, you're right.

Lukasa · 2018-05-16T19:42:11Z

Gotta fix those tests up, but I'm happy.

Motivation: In certain cases it's useful to quiesce a channel instead of just closing them immediately for example when receiving a signal. This lays the groundwork by introducing the `ChannelShouldQuiesceUserEvent` user event that when received can be interpreted by a ChannelHandler in a protocol & application specific way. Some protocols support tear down and that would be a good place to initiate the tear down. Modifications: - introduce `ChannelShouldQuiesceUserEvent` - handle `ChannelShouldQuiesceUserEvent` in the `AcceptHandler` with closing the server socket - handle `ChannelShouldQuiesceUserEvent` in the `HTTPServerPipelineHandler` by only handling a already in-flight request and then no longer accepting input - added `CircularBuffer.removeAll` (& tests) - added tests for `nextPowerOf2()` Result: - handlers can now support quiescing

weissi force-pushed the jw-signal-handling branch 2 times, most recently from 7d54102 to 5fac928 Compare May 14, 2018 15:12

Lukasa requested changes May 14, 2018

View reviewed changes

weissi force-pushed the jw-signal-handling branch from 5fac928 to 6fe5a6e Compare May 14, 2018 17:36

Lukasa requested changes May 15, 2018

View reviewed changes

weissi force-pushed the jw-signal-handling branch from 6fe5a6e to 5761eba Compare May 15, 2018 15:07

weissi changed the title ~~[DO NOT MERGE] WIP: signal handling~~ Channel quiescing support May 15, 2018

Lukasa requested changes May 15, 2018

View reviewed changes

weissi force-pushed the jw-signal-handling branch from 5761eba to 572c430 Compare May 15, 2018 15:24

normanmaurer reviewed May 15, 2018

View reviewed changes

normanmaurer requested changes May 15, 2018

View reviewed changes

weissi force-pushed the jw-signal-handling branch 2 times, most recently from c345489 to 23eedc9 Compare May 16, 2018 14:05

weissi commented May 16, 2018

View reviewed changes

weissi added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label May 16, 2018

weissi added this to the 1.7.0 milestone May 16, 2018

Lukasa requested changes May 16, 2018

View reviewed changes

Lukasa approved these changes May 16, 2018

View reviewed changes

weissi force-pushed the jw-signal-handling branch 2 times, most recently from f6250e4 to efd1029 Compare May 17, 2018 13:14

weissi force-pushed the jw-signal-handling branch from efd1029 to a49ba57 Compare May 17, 2018 13:14

normanmaurer approved these changes May 18, 2018

View reviewed changes

normanmaurer merged commit cea8476 into apple:master May 18, 2018

weissi deleted the jw-signal-handling branch May 19, 2018 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Channel quiescing support #399

Channel quiescing support #399

weissi commented May 9, 2018 •

edited

Lukasa May 14, 2018

Lukasa May 14, 2018

Lukasa May 14, 2018

Lukasa May 14, 2018

Lukasa May 14, 2018

Lukasa May 14, 2018

weissi May 14, 2018

weissi May 14, 2018

Lukasa May 14, 2018

Lukasa May 14, 2018

Lukasa May 15, 2018

BasThomas May 15, 2018

weissi May 15, 2018

Lukasa May 15, 2018

weissi May 15, 2018

weissi May 15, 2018

normanmaurer May 15, 2018

weissi May 15, 2018

normanmaurer May 15, 2018

weissi May 15, 2018

Lukasa May 16, 2018

normanmaurer May 15, 2018

weissi May 15, 2018

normanmaurer May 15, 2018

weissi May 15, 2018

normanmaurer May 16, 2018

weissi May 16, 2018

Lukasa May 16, 2018

weissi May 16, 2018

Lukasa May 16, 2018

Lukasa commented May 16, 2018

Channel quiescing support #399

Channel quiescing support #399

Conversation

weissi commented May 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented May 16, 2018

weissi commented May 9, 2018 •

edited