make Channel lifecycle statemachine explicit #220

weissi · 2018-03-22T18:05:34Z

@normanmaurer / @Lukasa this is a preview of what I'm working on. It passes tests, also happy with some monkey channel test which previously used to crash stuff a lot. But we should have a chat what we want to do in which situations. Also CC @vlm .

Motivation:

We had a lot of problems with the Channel lifecycle statemachine as it
wasn't explicit, this fixes this. Additionally, it asserts a lot more.

Modifications:

made Channel lifecycle statemachine explicit
lots of asserts

Result:

hopefully the state machine works better
the asserts should guide our way to work on in corner cases as well

Lukasa

Cool, the general shape of this is really good. Some notes inline.

Lukasa · 2018-03-23T08:49:26Z

Sources/NIO/BaseSocketChannel.swift

+                self.isActiveAtomic.store(false)
+            default:
+                ()
+            }


I assume the reason this conditional is a bit complex is to avoid doing too many atomic ops?

@Lukasa indeed, also don't think it's too complex tbh

Lukasa · 2018-03-23T08:50:55Z

Sources/NIO/BaseSocketChannel.swift

+        }
+    }
+
+    @inline(__always) // so we're not actually returning a closure


Would be nice to confirm that the compiler actually gets this right.

Presumably the other option is to pass a collection of non-escaping closures into this.

@normanmaurer / @Lukasa the problem here is that the compiler might change at any point in time and there's nothing that we can do to make sure that it's inlined unfortunately.

And passing the closures in is just so ugly, no?

Lukasa · 2018-03-23T08:51:42Z

Sources/NIO/BaseSocketChannel.swift

+
+        switch (self.currentState, event) {
+        // origin: .neverRegistered
+        case (.neverRegistered, .activate):


Do we intend to allow activating an unregistered socket? I think I'd kinda prefer we didn't.

Lukasa · 2018-03-23T08:53:23Z

Sources/NIO/BaseSocketChannel.swift

+            self.badTransition(event: event)
+
+        case (.registeredNeverActivated, .register):
+            self.badTransition(event: event)


Mild nit, but it may be a bit clearer to read this state machine if you fold all the badTransition cases for a given state together, e.g

case (.registeredNeverActivated, .deactivate), (.registeredNeverActivated, .register): self.badTransition(event: event)

Lukasa · 2018-03-23T08:55:04Z

Sources/NIO/BaseSocketChannel.swift

+    enum State {
+        case neverRegistered
+        case registeredNeverActivated
+        case registered


Hrm, do we want allow a channel to go registered -> active -> registered -> active? I'd be inclined to want Channels to be single-use constructs.

@Lukasa I think you are right... it should be single use.

Lukasa · 2018-03-23T08:57:15Z

Sources/NIO/BaseSocketChannel.swift

+        }
+    }
+
+    internal var isOpen: Bool {


This is such a weird property. I wonder if we really need it.

@Lukasa we use it all over the place to check if a channel has not been closed. Sure, I could introduce a isClosed property but then we'd need to guard !...isClosed which gives us the inversion always...

Lukasa · 2018-03-23T08:58:52Z

Sources/NIO/BaseSocketChannel.swift

-        self._pipeline = ChannelPipeline(channel: self)
+        self.lifecycleManager = SocketChannelLifecycleManager(eventLoop: self.eventLoop,
+                                                              channelPipeline: ChannelPipeline(channel: self),
+                                                              isActiveAtomic: Atomic(value: false))


We have a reference cycle here, right? lifecycleManager holds a reference to self, meaning self holds a cyclic reference to self.

@Lukasa no because ChannelPipeline holds Channel as an unowned reference. It's totally non-obvious but has always been like this.

Lukasa · 2018-03-23T09:01:09Z

Sources/NIO/BaseSocketChannel.swift

@@ -373,6 +561,11 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
            return
        }

+        guard self.isActive else {


This should probably check the lifecycleManager directly, as it's definitionally called from on the event loop.

thanks, yes, forgot to record that change

Lukasa · 2018-03-23T09:01:21Z

Sources/NIO/BaseSocketChannel.swift

@@ -410,7 +603,12 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {

        self.markFlushPoint(promise: nil)

+        guard self.isActive else {


Same note, we're on the event loop and can avoid the atomic.

Lukasa · 2018-03-23T09:02:07Z

Sources/NIO/BaseSocketChannel.swift

-        if !self.neverRegistered {
-            pipeline.fireChannelUnregistered0()
-        }
+        self.lifecycleManager.moveState(event: .close, promise: p)()


Do we want to wrap these in convenience methods that let the states and inputs become private to lifecycleManager? e.g. self.lifecycleManager.close().

ianpartridge · 2018-03-23T20:28:50Z

Sources/NIO/Channel.swift

@@ -278,6 +278,9 @@ public enum ChannelError: Error {

    /// A `DatagramChannel` `write` was made with an address that was not reachable and so could not be delivered.
    case writeHostUnreachable
+
+    /// An operation that was inappropriate given the current `Channel` state was attempted.
+    case inappropriateOperationForState


Does this need adding to the Equatable conformance?

thanks @ianpartridge indeed

normanmaurer

Just a few style changes... Also did you run a benchmark to verify how much more costly this is with all the closures ?

normanmaurer · 2018-03-26T17:05:47Z

Sources/NIO/BaseSocketChannel.swift

+    }
+
+    @inline(__always)
+    internal mutating func register(promise: EventLoopPromise<()>?) -> (() -> Void) {


@weissi Please change to use EventLoopPromise<Void> to be consistent

sorry, will do

normanmaurer · 2018-03-26T17:05:53Z

Sources/NIO/BaseSocketChannel.swift

+    }
+
+    @inline(__always)
+    internal mutating func close(promise: EventLoopPromise<()>?) -> (() -> Void) {


@weissi Please change to use EventLoopPromise<Void> to be consistent

normanmaurer · 2018-03-26T17:05:57Z

Sources/NIO/BaseSocketChannel.swift

+    }
+
+    @inline(__always)
+    internal mutating func activate(promise: EventLoopPromise<()>?) -> (() -> Void) {


@weissi Please change to use EventLoopPromise<Void> to be consistent

normanmaurer · 2018-03-26T17:06:02Z

Sources/NIO/BaseSocketChannel.swift

+
+    // MARK: private API
+    @inline(__always) // so we're not actually returning a closure
+    private mutating func moveState(event: Event, promise: EventLoopPromise<()>?) -> (() -> Void) {


@weissi Please change to use EventLoopPromise<Void> to be consistent

normanmaurer · 2018-03-26T17:09:54Z

Sources/NIO/BaseSocketChannel.swift

+    }
+
+    // MARK: private API
+    @inline(__always) // so we're not actually returning a closure


So just to clarify this is to safe the heap allocation ? If so can you just add this to the comment ?

Lukasa · 2018-03-26T16:28:52Z

Sources/NIO/BaseSocketChannel.swift

+    // MARK: API
+    internal init(eventLoop: EventLoop,
+                  channelPipeline: ChannelPipeline,
+                  isActiveAtomic: Atomic<Bool>) {


Is there a reason we're injecting this?

oh thanks, no there isn't anymore

@Lukasa now found the reason why this was injected: lifecycle manager is of course held mutable by the BaseSocketChannel. But we need to get to the atomic from any thread. Therefore the atomic needs to be stored on the BaseSocketChannel in a let and passed to the lifecycle manager. Sorry I did forget about that. Patch coming.

Lukasa · 2018-03-26T16:39:58Z

Sources/NIO/BaseSocketChannel.swift

+
+    // MARK: private API
+    @inline(__always) // so we're not actually returning a closure
+    private mutating func moveState(event: Event, promise: EventLoopPromise<()>?) -> (() -> Void) {


Are we still sure we don't heap-allocate a closure here?

let me read some SIL

@Lukasa ok I had a look at this and it seems totally fine. SIL didn't work as the compiler kept crashing, looked at the assembly instead. To make it easier I planted to bogus fcntl calls in there just so I know where we are:

and then I traced them back in the assembly which you can find below. I marked the beginning and the end of the interesting section as well as all the CALL instructions (which could lead to allocations):

as you can see all the calls are either retain or release, some optional projections or the call to badTransition which is expected. Do you agree that this is fine?

hang on, something's not quite right! I can't see the calls to pipeline.becomeActive and so either. LOoking

ok, it's slightly more complicated but the argument holds. Let me try to paste the right way through the code here:

sorry about that @Lukasa but this is the best I could do but there you can see quite well a run through of what happens:

the beginning (fcntl marker call)

fulfill the promise

fire channelActive

the end (fcntl marker call)

all the CALL instructions are highlighted and none of them should allocate. So it's all good I think

ah, in step 4 we can also see the atomic store (to mark the channel as active) which is also important but I forgot to annotate.

Lukasa · 2018-03-26T17:13:48Z

Sources/NIO/BaseSocketChannel.swift

            }

            // Call before triggering the close of the Channel.
-            pipeline.fireChannelReadComplete0()
+            if self.lifecycleManager.isActive {


Is this right? It's not clear to me that the contract of readComplete requires that it only fire on active channels. If it does, is that contract in tension with the contract that readComplete always fires after channelRead?

I think in general it should only fire when the channel is active.

@Lukasa, chatted to @normanmaurer and added a test which always gets into this situation by doing

one successful read

one read that fails with ECONNRESET

in the errorCaught calls ctx.close to close the channel

sees channelInactive (from the close)

does not ever see channelReadComplete.

normanmaurer · 2018-03-27T18:54:55Z

Sources/NIO/BaseSocketChannel.swift

+
+    // this is called from Channel's deinit, so don't assert we're on the EventLoop!
+    internal var canBeDestroyed: Bool {
+        if case .closed = self.currentState {


I think this can be simplified as:

return self.currentState == .closed

thanks @normanmaurer it can indeed now. Used to have more states :)

normanmaurer · 2018-03-27T18:56:16Z

Sources/NIO/BaseSocketChannel.swift

+    /// until the Channel is closed.
+    internal var isOpen: Bool {
+        assert(self.eventLoop.inEventLoop)
+        if case .closed = self.currentState {


Just do:

return self.currentState != .closed

normanmaurer · 2018-03-27T19:06:42Z

Sources/NIO/BaseSocketChannel.swift

+    internal let isActiveAtomic = Atomic(value: false)
+    // these are only to be accessed on the EventLoop
+    internal let channelPipeline: ChannelPipeline
+    private let eventLoop: EventLoop


Can't we remove this and just use channelPipeline.eventLoop when needed ?

normanmaurer · 2018-03-27T19:07:53Z

Sources/NIO/BaseSocketChannel.swift

+    private mutating func doStateTransfer(newState: State, promise: EventLoopPromise<Void>?, _ callouts: @escaping (ChannelPipeline) -> Void) -> (() -> Void) {
+        self.currentState = newState
+
+        let pipeline = self.channelPipeline


why we store the pipeline here first ? So we not escape self ?

@normanmaurer yes so that self isn't captured. I can remove now as the @inline(_always) should make the whole closure go away anyway. Want me to?

normanmaurer · 2018-03-27T19:08:36Z

Sources/NIO/BaseSocketChannel.swift

+            return false
+        case .activated:
+            return true
+        }


why not just use?:

return self.currentState == .activated

thanks, same here, used to be more states

normanmaurer · 2018-03-28T18:22:28Z

@weissi please fix the test compile error.

weissi · 2018-03-29T09:59:33Z

@normanmaurer sorry, should be fixed

normanmaurer · 2018-03-29T17:12:52Z

@weissi can you also run some benchmarks with this ?

weissi · 2018-03-29T17:22:47Z

@normanmaurer the same:

before:

GO
Destination: [127.0.0.1]:9999
Interface lo0 address [127.0.0.1]:0
Using interface lo0 to connect to [127.0.0.1]:9999
Ramped up to 10 connections.
Total data sent:     32351.8 MiB (33923309052 bytes)
Total data received: 32347.9 MiB (33919252327 bytes)
Bandwidth per channel: 2713.545⇅ Mbps (339193.1 kBps)
Aggregate bandwidth: 13566.911↓, 13568.534↑ Mbps
Packet rate estimate: 1208692.7↓, 1194076.8↑ (11↓, 31↑ TCP MSS/op)
Test duration: 20.0012 s.

after:

GO
Destination: [127.0.0.1]:9999
Interface lo0 address [127.0.0.1]:0
Using interface lo0 to connect to [127.0.0.1]:9999
Ramped up to 10 connections.
Total data sent:     32794.0 MiB (34387041676 bytes)
Total data received: 32788.0 MiB (34380754909 bytes)
Bandwidth per channel: 2750.181⇅ Mbps (343772.6 kBps)
Aggregate bandwidth: 13749.648↓, 13752.162↑ Mbps
Packet rate estimate: 1217976.4↓, 1214771.9↑ (11↓, 32↑ TCP MSS/op)
Test duration: 20.0039 s.

Lukasa

LGTM, ship it.

Motivation: We had a lot of problems with the Channel lifecycle statemachine as it wasn't explicit, this fixes this. Additionally, it asserts a lot more. Modifications: - made Channel lifecycle statemachine explicit - lots of asserts Result: - hopefully the state machine works better - the asserts should guide our way to work on in corner cases as well

normanmaurer · 2018-03-29T17:50:13Z

@weissi thanks ... merged!

weissi requested review from normanmaurer and Lukasa March 22, 2018 18:05

weissi added the 🔨 semver/patch No public API change. label Mar 22, 2018

Lukasa requested changes Mar 23, 2018

View reviewed changes

ianpartridge reviewed Mar 23, 2018

View reviewed changes

weissi force-pushed the jw-channel-lifecycle-statemachine branch 2 times, most recently from 3f920cb to f1c810e Compare March 26, 2018 15:47

normanmaurer requested changes Mar 26, 2018

View reviewed changes

Lukasa requested changes Mar 26, 2018

View reviewed changes

weissi force-pushed the jw-channel-lifecycle-statemachine branch 2 times, most recently from 9f109d8 to 87ba71c Compare March 27, 2018 18:04

normanmaurer requested changes Mar 27, 2018

View reviewed changes

normanmaurer reviewed Mar 27, 2018

View reviewed changes

weissi added 🆕 semver/minor Adds new public API. and removed 🔨 semver/patch No public API change. labels Mar 28, 2018

weissi force-pushed the jw-channel-lifecycle-statemachine branch 3 times, most recently from 985466d to 20ea88f Compare March 28, 2018 16:08

weissi force-pushed the jw-channel-lifecycle-statemachine branch from 20ea88f to 7abbb03 Compare March 29, 2018 09:58

normanmaurer approved these changes Mar 29, 2018

View reviewed changes

Lukasa approved these changes Mar 29, 2018

View reviewed changes

Lukasa added this to the 1.4.0 milestone Mar 29, 2018

weissi force-pushed the jw-channel-lifecycle-statemachine branch from 5cc3082 to b1375ac Compare March 29, 2018 17:46

normanmaurer merged commit 2374421 into apple:master Mar 29, 2018

weissi deleted the jw-channel-lifecycle-statemachine branch April 2, 2018 11:23

BasThomas mentioned this pull request Apr 11, 2018

New assert failure with 1.4.0 #302

Closed

Lukasa mentioned this pull request Apr 12, 2018

Guard against re-entrance of close0 #239

Closed

		@@ -410,7 +603,12 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {

		self.markFlushPoint(promise: nil)

		guard self.isActive else {

make Channel lifecycle statemachine explicit #220

make Channel lifecycle statemachine explicit #220

Conversation

weissi commented Mar 22, 2018 • edited Loading

Lukasa left a 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

normanmaurer left a 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

weissi Apr 9, 2018 • edited Loading

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

weissi Mar 27, 2018 • edited Loading

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

normanmaurer Mar 27, 2018 • edited Loading

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

normanmaurer commented Mar 28, 2018

weissi commented Mar 29, 2018

normanmaurer commented Mar 29, 2018

weissi commented Mar 29, 2018

Lukasa left a comment

Choose a reason for hiding this comment

normanmaurer commented Mar 29, 2018

weissi commented Mar 22, 2018 •

edited

Loading

weissi Apr 9, 2018 •

edited

Loading

weissi Mar 27, 2018 •

edited

Loading

normanmaurer Mar 27, 2018 •

edited

Loading