-
Notifications
You must be signed in to change notification settings - Fork 652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement lazy selector registration #388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, mostly looks really good! Some notes!
Sources/NIO/BaseSocketChannel.swift
Outdated
@@ -18,14 +18,16 @@ private struct SocketChannelLifecycleManager { | |||
// MARK: Types | |||
private enum State { | |||
case fresh | |||
case registered | |||
case lazilyRegistered // register() has been run but the selector doesn't know about it yet | |||
case fullyRegistered // fully registered, ie. the selector knows about it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I don't love these names. Can we call the new state preRegistered
, maybe?
Sources/NIO/BaseSocketChannel.swift
Outdated
case activated | ||
case closed | ||
} | ||
|
||
private enum Event { | ||
case activate | ||
case register | ||
case registerLazily | ||
case registerFully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, these names aren't necessarily right. Maybe beginRegistration
and completeRegistration
?
Sources/NIO/BaseSocketChannel.swift
Outdated
if self.lifecycleManager.isRegisteredLazily { | ||
try! becomeFullyRegistered0() | ||
if self.lifecycleManager.isRegisteredFully { | ||
becomeActive0(promise: promise) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these callouts not get triggered by register0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lukasa right now register0
only does the pre-registration. I didn't change its name as it's on ChannelCore
. But we could obviously rename it to preRegister0
Sources/NIO/BaseSocketChannel.swift
Outdated
guard self.selectableEventLoop.isOpen else { | ||
let error = EventLoopError.shutdown | ||
self.pipeline.fireErrorCaught0(error: error) | ||
self.close0(error: error, mode: .all, promise: nil) | ||
promise?.fail(error: error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just pass the promise
into self.close0(...)
which will ensure correct ordering as well.
So basically:
self.close0(error: error, mode: .all, promise: promise)
Sources/NIO/EventLoop.swift
Outdated
@@ -337,6 +337,11 @@ internal final class SelectableEventLoop: EventLoop { | |||
_addresses.deallocate() | |||
} | |||
|
|||
/// Is this `SelectableEventLoop` still open (ie. not shutting down or shut down) | |||
public var isOpen: Bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weissi this is not safe as lifecycleState
is not thread-safe. Maybe change to internal and assert that the caller is the EventLoop thread ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@normanmaurer it's fine as SelectableEventLoop
is an internal
type (not public) but regardless I changed this to internal
and added an assertion
Tests/NIOTests/ChannelTests.swift
Outdated
XCTAssertNoThrow(try server.register().wait()) | ||
XCTAssertNoThrow(try server.eventLoop.submit { | ||
XCTAssertFalse(server.isActive) | ||
}.wait()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove 4 spaces.
@@ -2317,6 +2317,43 @@ public class ChannelTests: XCTestCase { | |||
|
|||
} | |||
|
|||
func testLazyRegistrationWorksForServerSockets() throws { | |||
let group = MultiThreadedEventLoopGroup(numThreads: 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add:
defer {
XCTAssertNoThrow(try group.syncShutdownGracefully())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
} | ||
|
||
func testLazyRegistrationWorksForClientSockets() throws { | ||
let group = MultiThreadedEventLoopGroup(numThreads: 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add:
defer {
XCTAssertNoThrow(try group.syncShutdownGracefully())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Motivation: Previously we would eagerly register any fd with the selector (epoll/kqueue) which works well with kqueue. With epoll however, you'll get `EPOLLHUP` immediately if you supplied a socket that hasn't had connect/bind called on it. We got away with this because we took quite a bit of care to always make sure we call connect/bind pretty much immediately after we registered it. And crucially before we ever asked the selector to tell us about new events. Modifications: made selector registration lazy (when we try activating the channel by calling connect/bind) Result: the user can now register and connect whenever they feel like it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, great job @weissi!
Thanks @weissi ... merged :) |
Motivation:
Previously we would eagerly register any fd with the selector
(epoll/kqueue) which works well with kqueue. With epoll however, you'll
get
EPOLLHUP
immediately if you supplied a socket that hasn't hadconnect/bind called on it.
We got away with this because we took quite a bit of care to always make
sure we call connect/bind pretty much immediately after we registered
it. And crucially before we ever asked the selector to tell us about
new events.
Modifications:
made selector registration lazy (when we try activating the channel by
calling connect/bind)
Result:
the user can now register and connect whenever they feel like it, fixes #380