-
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
Ensure correct ordering of promise notification when connect is still… #330
Ensure correct ordering of promise notification when connect is still… #330
Conversation
2ee9f80
to
59da341
Compare
Sources/NIO/BaseSocketChannel.swift
Outdated
@@ -677,6 +677,11 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore { | |||
// Transition our internal state. | |||
let callouts = self.lifecycleManager.close(promise: p) | |||
|
|||
if let connectPromise = pendingConnect { |
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.
Now that we're touching this, can we add the missing self
clauses?
} | ||
|
||
class NotificationOrderHandler: ChannelDuplexHandler { | ||
typealias InboundIn = Any |
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.
Better to make these Never
?
let group = MultiThreadedEventLoopGroup(numThreads: 1) | ||
defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } | ||
|
||
let channel = try SocketChannel(eventLoop: group.next() as! SelectableEventLoop, protocolFamily: PF_INET) |
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 isn't the constructor you want: right now you're using an actual socket, not the ConnectPendingSocket
.
let connectPromise: EventLoopPromise<Void> = channel.eventLoop.newPromise() | ||
let closePromise: EventLoopPromise<Void> = channel.eventLoop.newPromise() | ||
|
||
channel.connect(to: try! SocketAddress.init(ipAddress: "127.0.0.1", port: 9999), promise: connectPromise) |
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.
extra .init
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.
move try
to the beginning of the line and add XCTAssertNoThrow(...)
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 I just removed the !
which should be good enough as it will fail the test.
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.
looks pretty good, few nits apart from what Cory wrote
74aeddb
to
abc35d2
Compare
let channel = try SocketChannel(socket: ConnectPendingSocket(promise: promise), parent: nil, eventLoop: eventLoop as! SelectableEventLoop) | ||
|
||
XCTAssertNoThrow(try channel.pipeline.add(handler: NotificationOrderHandler()).then { | ||
channel.register() |
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.
You can't do this, it'll cause the channel to be insta-closed because we'll register the channel and then immediately get EPOLLHUP.
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.
Hmm what you suggest to do here to test this ?
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.
Also it works me be preferable to make this consistent on Mac and Linux as on Mac it works
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.
We cannot make this consistent on Mac and Linux, really, because to do it requires enormously annoying insight into the socket state. Specifically, it would require that the kqueue selector can check for all FDs registered with it whether they're connected or not before it enters the selector, which is obviously untenable.
The other solution is to have the epoll selector refuse to actually register anything until something registers for either readable or writable. That's more tenable, but the code sucks.
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 hmm ok... so any idea for writing a test that works on mac and linux ?
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 the fix is correct and the test shows it on Mac but would love to have a test that works on both ;)
abc35d2
to
6a95fe9
Compare
// The close needs to happen in the then { ... } block to ensure we close the channel | ||
// before we have the chance to register it for .write. | ||
channel.close(promise: closePromise) | ||
return channel.eventLoop.newSucceededFuture(result: ()) |
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.
Why are you using then
and returning succeeded futures? Just use map
with a void return type. 😄
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.
🤦♂️
7888792
to
e7cd1fa
Compare
@Lukasa addressed and rebased, |
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.
You uh...you didn't change anything.
@Lukasa huh ? https://github.com/apple/swift-nio/pull/330/files#diff-570b896bf499f64d3df962dd1026c797R432 ? I changed the |
// We need to call submit {...} here to ensure then {...} is called while on the EventLoop already to not have | ||
// a ECONNRESET sneak in. | ||
XCTAssertNoThrow(try channel.eventLoop.submit { | ||
channel.register().then { () -> EventLoopFuture<()> in |
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.
There's a then right here.
channel.register().then { () -> EventLoopFuture<()> in | ||
channel.connect(to: serverChannel.localAddress!, promise: connectPromise) | ||
return channel.eventLoop.newSucceededFuture(result: ()) | ||
}.map { () -> () in |
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.
Also our style is to spell the second parenthesis pair here as Void
(that is, () -> Void
), and if you've done that you don't need the explicit return
statement.
e7cd1fa
to
b348351
Compare
LGTM, we can wait for @weissi. |
Sounds good... |
// before we have the chance to register it for .write. | ||
channel.close(promise: closePromise) | ||
} | ||
}.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.
this needs an extra .wait() as Void
(see #332)
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 fixed please check again
… pending. Motivation: When a pending connect is still in process and the Channel is closed we need to ensure we use the correct ordering when notify the promises and ChannelHandlers in the pipeline. The ordering should be: - Notify pending connect promise - Notify close promise - Call channelInactive(...) Modifications: - Correct ordering of notification - Add unit test Result: Ensure correct ordering when connect is still pending
b348351
to
7f87322
Compare
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.
👍 looks cool if tests pass
… pending.
Motivation:
When a pending connect is still in process and the Channel is closed we need to ensure we use the correct ordering when notify the promises and ChannelHandlers in the pipeline.
The ordering should be:
Modifications:
Result:
Ensure correct ordering when connect is still pending