don't deliver events for unregistered fds #341

weissi · 2018-04-19T18:50:35Z

Motivation:

Since forever we had a major bug in the Selector: In this condition:

kqueue/epoll had many events
in one of the earlier events we unregister a Channel whose fd is on of
the later events
we subsequently (still in the same event loop tick) register a new
channel which gets the same fd as the previously closed one

then we would deliver an event that was meant for a previous channel to
a newly opened one.

Thanks to @mcdappdev for hitting this bug, helping us debug it and also
providing a repeatedly working repro.

Modifications:

if during event delivery any fd gets unregistered, we stop delivering
the remaining events and rely on the selector to redeliver them
again next time.

Result:

we don't deliver events for previously closed channels to new ones.

normanmaurer · 2018-04-19T18:56:40Z

Sources/NIO/Selector.swift

@@ -540,7 +543,8 @@ final class Selector<R: Registration> {
            Int(try KQueue.kevent(kq: self.fd, changelist: nil, nchanges: 0, eventlist: events, nevents: Int32(eventsCapacity), timeout: ts))
        }

-        for i in 0..<ready {
+        self.deregistrationsHappened = false
+        for i in 0..<ready where !self.deregistrationsHappened {


as noted offline I think while this should work we should treat this as workaround as this depends highly on the "level-triggered" semantics of kqueue / epoll.

Agreed. This should be thought of as a temporary fix to avoid the nasty behaviour: we should come up with a better long-term solution for dealing with this in a future release.

Totally agreed. Let’s get this in (I actually found another bug (and a missing assertion) whilst fixing it) and then fix properly. I agree we shouldn’t rely on it being level triggered here.

Works for me

Lukasa · 2018-04-20T08:28:59Z

Sources/NIO/Selector.swift

@@ -259,6 +259,7 @@ final class Selector<R: Registration> {
    private var eventsCapacity = 64
    private var events: UnsafeMutablePointer<EventType>
    private var registrations = [Int: R]()
+    private var deregistrationsHappened: Bool = false


Can you put a doc comment on this guy indicating that it's a temporary fix?

Lukasa · 2018-04-20T08:36:39Z

Sources/NIO/Selector.swift

@@ -500,7 +502,8 @@ final class Selector<R: Registration> {
            ready = Int(try Epoll.epoll_wait(epfd: self.fd, events: events, maxevents: Int32(eventsCapacity), timeout: -1))
        }

-        for i in 0..<ready {
+        self.deregistrationsHappened = false


I actually don't think we need to do this for epoll. I realised this this morning, but epoll generally only returns each FD once, with a mask of all event flags for that FD.

This means that our epoll code is immune to this bug: the channel is concretely held to dispatch all the events to it. This also suggests a sensible long-term fix: we should coalesce the kqueue notifications so that each channel has events dispatched to it only once.

That’s actually a very good point 👍

that's not true because (just like the test does), if you close lots of fds and then open lots of fds, there might very well be events for file descriptors that are no longer valid.

Yeah, you're right.

doh... true :(

Lukasa

Cool, mostly looks really good. Some notes inline.

Lukasa · 2018-04-20T12:01:15Z

Sources/NIO/BaseSocketChannel.swift

@@ -776,6 +781,9 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
    }

    final func readEOF() {
+        print("readEOF \(self)")


Print statement left behind.

Lukasa · 2018-04-20T12:01:48Z

Sources/NIO/Selector.swift

@@ -259,6 +259,7 @@ final class Selector<R: Registration> {
    private var eventsCapacity = 64
    private var events: UnsafeMutablePointer<EventType>
    private var registrations = [Int: R]()
+    private var deregistrationsHappened: Bool = false


Lukasa · 2018-04-20T12:04:37Z

Tests/NIOTests/SelectorTest.swift

+                // impossible to get a read notification in the very same event loop tick that you got registered
+                XCTAssertTrue(self.hasReConnectEventLoopTickFinished.value)
+
+                // TODO: check the bytebuffer


Lukasa · 2018-04-20T12:05:05Z

Tests/NIOTests/SelectorTest.swift

+            }
+
+            func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
+                // we expect these channels to be get data only a while after the re-connect event loop tick as it's


s/to be get/to get/

Lukasa · 2018-04-20T12:07:02Z

Tests/NIOTests/SelectorTest.swift

+            func userInboundEventTriggered(ctx: ChannelHandlerContext, event: Any) {
+                // this is the `.readEOF` that is triggered by the `ServerHandler`'s `close` calls because our channel
+                // supports half-closure
+                if self.allChannels.value.count == SelectorTest.testWeDoNotDeliverEventsForPreviouslyClosedChannels_numberOfChannelsToUse {


This would be cleaner as guard.

Lukasa · 2018-04-20T12:09:04Z

Tests/NIOTests/SelectorTest.swift

+                                               Note: if you changed anything about the pipeline's handler adding/removal
+                                               you might also have a bug there.
+                                               """)
+                                return ()


Shouldn't need the return statement.

normanmaurer · 2018-04-20T12:06:12Z

Sources/NIO/BaseSocketChannel.swift

@@ -776,6 +781,9 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
    }

    final func readEOF() {
+        print("readEOF \(self)")


@weissi remove

normanmaurer · 2018-04-20T12:08:03Z

Tests/NIOTests/SelectorTest.swift

+    private static let testWeDoNotDeliverEventsForPreviouslyClosedChannels_numberOfChannelsToUse = 10
+    func testWeDoNotDeliverEventsForPreviouslyClosedChannels() {
+        /// We use this class to box mutable values, generally in this test anything boxed should only be read/written
+        /// on the event loop `el`.


@weissi should we use a computed property and check MultiThreadedEventLoopThread.currentEventLoop returns what we expect ?

sure why not

normanmaurer · 2018-04-20T12:08:52Z

Tests/NIOTests/SelectorTest.swift

+            case didNotReadGotReadComplete
+        }
+
+        /// This handler is inserted in the channels that are re-connected. So we're closing a bunch of channels and


... is inserted in the ChannelPipeline of the channels that are re-connected

normanmaurer · 2018-04-20T12:09:17Z

Tests/NIOTests/SelectorTest.swift

+
+            private let didReadPromise: EventLoopPromise<Void>
+            private var didRead: Bool = false
+            private let hasReConnectEventLoopTickFinished: Box<Bool>


can we group the let together ?

normanmaurer · 2018-04-20T12:10:38Z

Tests/NIOTests/SelectorTest.swift

+        }
+        let el = MultiThreadedEventLoopGroup(numThreads: 1).next()
+        defer {
+            try! el.syncShutdownGracefully()


XCTAssertNoThrow(...) ?

normanmaurer · 2018-04-20T12:11:13Z

Tests/NIOTests/TestUtils.swift

+        }
+    }
+    templateBytes.removeLast()
+    let udsTempDir = String(decoding: templateBytes, as: UTF8.self)


nit: just return.. not net to create the let first.

weissi · 2018-04-20T13:05:43Z

@normanmaurer / @Lukasa all addressed

Lukasa · 2018-04-20T14:22:34Z

Sources/NIO/BaseSocketChannel.swift

        assert(eventLoop.inEventLoop)
        assert(self.lifecycleManager.isRegistered)

+        guard !self.lifecycleManager.hasSeenEOFNotification else {


Should this be a guard or an assert?

@Lukasa this will be run if the user does ctx.read()

Lukasa · 2018-04-20T14:24:40Z

Tests/NIOTests/EchoServerClientTest.swift

+                channel.pipeline.add(handler: countingHandler)
+            }
+            .bind(host: "127.0.0.1", port: 0)
+            .thenIfError {


Surely bind isn't going to error.

normanmaurer

Looks great!

@mcdappdev

Motivation: Since forever we had a major bug in the Selector: In this condition: - kqueue/epoll had many events - in one of the earlier events we unregister a Channel whose fd is on of the later events - we subsequently (still in the same event loop tick) register a new channel which gets the same fd as the previously closed one then we would deliver an event that was meant for a previous channel to a newly opened one. Thanks to @mcdappdev for hitting this bug, helping us debug it and also providing a repeatedly working repro. Modifications: if during event delivery any fd gets unregistered, we stop delivering the remaining events and rely on the selector to redeliver them again next time. Result: we don't deliver events for previously closed channels to new ones.

weissi · 2018-04-20T15:18:49Z

did over the shoulder review with @Lukasa who is happy, so pressing the 'dismiss review' button :)

because he's happy

normanmaurer reviewed Apr 19, 2018

View reviewed changes

weissi added this to the 1.5.1 milestone Apr 19, 2018

weissi force-pushed the jw-fix-delivery-of-unregistered-fds branch from 27b4ff0 to 9d951f1 Compare April 19, 2018 20:32

Lukasa requested changes Apr 20, 2018

View reviewed changes

weissi force-pushed the jw-fix-delivery-of-unregistered-fds branch from 9d951f1 to 755bc7a Compare April 20, 2018 11:48

Lukasa requested changes Apr 20, 2018

View reviewed changes

normanmaurer requested changes Apr 20, 2018

View reviewed changes

weissi force-pushed the jw-fix-delivery-of-unregistered-fds branch from 755bc7a to f57010d Compare April 20, 2018 13:05

weissi force-pushed the jw-fix-delivery-of-unregistered-fds branch 2 times, most recently from 7f1cadf to c7c8b6f Compare April 20, 2018 14:05

Lukasa previously requested changes Apr 20, 2018

View reviewed changes

normanmaurer approved these changes Apr 20, 2018

View reviewed changes

weissi force-pushed the jw-fix-delivery-of-unregistered-fds branch from c7c8b6f to 01b5e71 Compare April 20, 2018 14:33

weissi merged commit 902b18d into apple:master Apr 20, 2018

weissi deleted the jw-fix-delivery-of-unregistered-fds branch April 20, 2018 15:19

Lukasa added the 🔨 semver/patch No public API change. label Apr 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

don't deliver events for unregistered fds #341

don't deliver events for unregistered fds #341

weissi commented Apr 19, 2018 •

edited

Loading

normanmaurer Apr 19, 2018

Lukasa Apr 19, 2018

weissi Apr 19, 2018

normanmaurer Apr 19, 2018

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

normanmaurer Apr 20, 2018

weissi Apr 20, 2018

Lukasa Apr 20, 2018

normanmaurer Apr 20, 2018

Lukasa left a comment

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

Lukasa Apr 20, 2018

weissi Apr 20, 2018

Lukasa Apr 20, 2018

normanmaurer Apr 20, 2018

normanmaurer Apr 20, 2018

weissi Apr 20, 2018

normanmaurer Apr 20, 2018

normanmaurer Apr 20, 2018

normanmaurer Apr 20, 2018

normanmaurer Apr 20, 2018

weissi commented Apr 20, 2018

Lukasa Apr 20, 2018

weissi Apr 20, 2018

Lukasa Apr 20, 2018

weissi Apr 20, 2018

normanmaurer left a comment

weissi commented Apr 20, 2018

don't deliver events for unregistered fds #341

don't deliver events for unregistered fds #341

Conversation

weissi commented Apr 19, 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

Choose a reason for hiding this comment

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

weissi commented Apr 20, 2018

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 Apr 20, 2018

weissi commented Apr 19, 2018 •

edited

Loading