channelActive needs to happen before channelRead #204

weissi · 2018-03-20T17:08:02Z

Many thanks for @vlm for the repro! Fixes #196

Motivation:

readIfNeeded was triggered straight after the channel was registered
but before channelActive was called which is wrong.

Modifications:

Moved the calls to readIfNeeded into becomeActive0 and removed them
everywhere else (except some error path).

Result:

handle ChannelHandler lifecycle more correctly.

normanmaurer

LGTM in general but needs a test

normanmaurer · 2018-03-21T13:58:06Z

Tests/NIOTests/ChannelTests.swift

+            }
+
+            func channelActive(ctx: ChannelHandlerContext) {
+                var buffer = ctx.channel.allocator.buffer(capacity: 100)


nit: just allocate 4 as this is big enough for "foo" ;)

but what if it's in UTF-264 encoding that I'll announce tomorrow? :p

fixing :)

normanmaurer · 2018-03-21T13:59:02Z

Tests/NIOTests/ChannelTests.swift

+                // this will fail as the socket is already pre-connected ;)
+                sc.connect(to: bootstrap.localAddress!).map {
+                    XCTFail("should've failed")
+                    }.mapIfError { error in


the indention doesn't look right...

normanmaurer · 2018-03-21T13:59:49Z

Tests/NIOTests/ChannelTests.swift

+            }
+        }
+
+        let elg = MultiThreadedEventLoopGroup(numThreads: 1)


also shut this down in a defer block

normanmaurer · 2018-03-21T14:02:11Z

Tests/NIOTests/ChannelTests.swift

+
+            private let writeDonePromise: EventLoopPromise<()>
+
+            init(writeDonePromise: EventLoopPromise<()>) {


@weissi we usually use EventLoopPromise<Void> I think... Not sure what is the "more swifty way tho". Thoughts ?

@normanmaurer for functions that have 'no return value' (sure you could return () but you don't have to) I tend to use -> Void but the promises we succeed with the value (). Therefore I mostly use EventLoopPromise<()> as there is a value 'sent'. Our codebase has both:

$ git grep 'EventLoopPromise<Void>' | wc -l 220 $ git grep 'EventLoopPromise<()>' | wc -l 57 $ git grep 'EventLoopFuture<()>' | wc -l 31 $ git grep 'EventLoopFuture<Void>' | wc -l 69

@weissi maybe a follow-up but I think w should make it consistent ;) I am not sure what is the "preferred" way tho.

@normanmaurer ok, I'll file an issue.

normanmaurer · 2018-03-21T14:03:31Z

Tests/NIOTests/ChannelTests.swift

+        defer {
+            XCTAssertNoThrow(try elg.syncShutdownGracefully())
+        }
+        let sc = try SocketChannel(eventLoop: elg.next() as! SelectableEventLoop, protocolFamily: AF_INET)


IMHO we use PF_INET usually, so maybe change this as well ?

AF_INET is newer, more standard and overall more preferred. https://stackoverflow.com/questions/6729366/what-is-the-difference-between-af-inet-and-pf-inet-in-socket-programming

normanmaurer · 2018-03-21T14:04:32Z

Tests/NIOTests/ChannelTests.swift

+
+        let serverWriteHappenedPromise: EventLoopPromise<()> = elg.next().newPromise()
+        let clientHasRegistered: EventLoopPromise<()> = elg.next().newPromise()
+        let clientHasUnregistered: EventLoopPromise<()> = elg.next().newPromise()


same comment about using Void here.

normanmaurer · 2018-03-21T14:04:57Z

Tests/NIOTests/ChannelTests.swift

+        let clientHasRegistered: EventLoopPromise<()> = elg.next().newPromise()
+        let clientHasUnregistered: EventLoopPromise<()> = elg.next().newPromise()
+
+        let bootstrap = try! ServerBootstrap(group: elg)


I think you can remove ! here.

normanmaurer

A few more comments

Lukasa · 2018-03-21T14:29:20Z

Tests/NIOTests/ChannelTests.swift

+                }
+            }.wait()
+        try clientHasRegistered.futureResult.wait()
+        try sc.close().wait()


I think this is very slightly probabilistic, as the close can technically race with the return to the selector such that it occurs before the selector is next polled. I think the only way you can avoid this is to connect-and-read-from another channel, which kinda sucks, but may be necessary.

normanmaurer · 2018-03-21T15:43:02Z

Tests/NIOTests/ChannelTests.swift

+        }
+        let serverEL = group.next()
+        let clientEL = group.next()
+        precondition(serverEL !== clientEL)


@weissi just make this an XCTAssertTrue ?

the problem is that if I do this, the test would just hang forever if something goes wrong (as XCTAsserts trigger after the test has finished)

I see... ok

normanmaurer · 2018-03-21T15:44:53Z

Tests/NIOTests/ChannelTests.swift

+                sc.register()
+            }.then {
+                // this should fail (and will on Darwin but _not_ Linux) as the socket is already pre-connected ;)
+                // on Linux a subsequent connect will just silently succeed, go figure...


This really makes me sad :( I wonder if there is no other way to test this in a more "consistent" way

@normanmaurer after @Lukasa 's patch is in I can create a Socket subclass which lets a subsequent 'special' connect succeed on all platforms. That way we can test even more as the channel does become active.

@weissi which patch you are talking about ? :)

@normanmaurer I must have dreamt ;)

vlm · 2018-03-21T16:10:35Z

Tests/NIOTests/ChannelTests.swift

+        let serverEL = group.next()
+        let clientEL = group.next()
+        precondition(serverEL !== clientEL)
+        let sc = try SocketChannel(eventLoop: clientEL as! SelectableEventLoop, protocolFamily: PF_INET)


I'd prefer AF_INET. 2c.

isn't AF address family and PF protocol family?

They're the same. There's no meaningful distinction here: the idea that these constants are different is a legacy from a bygone era.

@Lukasa so should I use PF_INET | AF_INET :P

@normanmaurer wanted PF_INET #204 (comment) . I don't care :)

@weissi @Lukasa to be clear I dont care which we use as long as we use it in a consistent way ;) I only suggested PF_INET because we use it in SocketAddress.

Lukasa · 2018-03-21T16:20:16Z

Tests/NIOTests/ChannelTests.swift

+                // available to be read immediately
+                sc.register()
+            }.then {
+                // this should fail (and will on Darwin but _not_ Linux) as the socket is already pre-connected ;)


This comment is out of date.

thanks, fixed

Motivation: `readIfNeeded` was triggered straight after the channel was registered but _before_ `channelActive` was called which is wrong. Modifications: Moved the calls to `readIfNeeded` into `becomeActive0` and removed them everywhere else (except some error path). Result: handle ChannelHandler lifecycle more correctly.

normanmaurer

LGTM if it passes the CI :)

Motivation: As Chrome has started GREASEing HTTP/2 settings and frames, we should ensure that we have tests that explicitly validate that we tolerate GREASEing these parameters. A future version of swift-nio-http2 should start emitting GREASE as well, but for now we'll satisfy ourselves with this. Two relevant citations: https://tools.ietf.org/html/draft-bishop-httpbis-grease-00 https://bugs.chromium.org/p/chromium/issues/detail?id=1083706 Modifications: - Added a frame parser test - Added a end-to-end settings test. Result: Validated that GREASE is tolerated.

weissi changed the title ~~channelActive needs to happen before channelRead~~ [WIP-TEST-MISSING] channelActive needs to happen before channelRead Mar 20, 2018

normanmaurer requested changes Mar 20, 2018

View reviewed changes

vlm mentioned this pull request Mar 21, 2018

channelRead invoked before channelActive #196

Open

weissi force-pushed the jw-read-after-active-not-registered branch 2 times, most recently from 2b09023 to 4ca8ae4 Compare March 21, 2018 13:47

weissi changed the title ~~[WIP-TEST-MISSING] channelActive needs to happen before channelRead~~ channelActive needs to happen before channelRead Mar 21, 2018

normanmaurer reviewed Mar 21, 2018

View reviewed changes

weissi force-pushed the jw-read-after-active-not-registered branch from 4ca8ae4 to c72bac4 Compare March 21, 2018 14:00

normanmaurer reviewed Mar 21, 2018

View reviewed changes

normanmaurer requested changes Mar 21, 2018

View reviewed changes

weissi force-pushed the jw-read-after-active-not-registered branch 2 times, most recently from 4c2ff85 to 0d001f6 Compare March 21, 2018 14:06

Lukasa requested changes Mar 21, 2018

View reviewed changes

weissi force-pushed the jw-read-after-active-not-registered branch 3 times, most recently from 3339426 to 3993ca3 Compare March 21, 2018 15:43

normanmaurer requested changes Mar 21, 2018

View reviewed changes

vlm reviewed Mar 21, 2018

View reviewed changes

weissi force-pushed the jw-read-after-active-not-registered branch from 3993ca3 to 577dc76 Compare March 21, 2018 16:17

Lukasa reviewed Mar 21, 2018

View reviewed changes

weissi force-pushed the jw-read-after-active-not-registered branch from 577dc76 to a495ac0 Compare March 21, 2018 16:22

normanmaurer approved these changes Mar 21, 2018

View reviewed changes

Lukasa approved these changes Mar 21, 2018

View reviewed changes

Lukasa merged commit 24fbb1b into apple:master Mar 21, 2018

normanmaurer added this to the 1.3.0 milestone Mar 21, 2018

normanmaurer added the 🔨 semver/patch No public API change. label Mar 21, 2018

weissi deleted the jw-read-after-active-not-registered branch July 9, 2024 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

channelActive needs to happen before channelRead #204

channelActive needs to happen before channelRead #204

weissi commented Mar 20, 2018 •

edited

Loading

normanmaurer left a comment

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

normanmaurer Mar 21, 2018

normanmaurer Mar 21, 2018

weissi Mar 21, 2018 •

edited

Loading

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

vlm Mar 21, 2018

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

normanmaurer left a comment

Lukasa Mar 21, 2018

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

weissi Mar 21, 2018

vlm Mar 21, 2018

weissi Mar 21, 2018

Lukasa Mar 21, 2018

weissi Mar 21, 2018

weissi Mar 21, 2018

normanmaurer Mar 21, 2018

Lukasa Mar 21, 2018

weissi Mar 21, 2018

normanmaurer left a comment


		private let writeDonePromise: EventLoopPromise<()>

		init(writeDonePromise: EventLoopPromise<()>) {

channelActive needs to happen before channelRead #204

channelActive needs to happen before channelRead #204

Conversation

weissi commented Mar 20, 2018 • edited Loading

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

weissi Mar 21, 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

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

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

weissi commented Mar 20, 2018 •

edited

Loading

weissi Mar 21, 2018 •

edited

Loading