Synchronous connection failures must close channels. #329
Conversation
Lukasa
added
the
patch-version-bump-only
Sources/NIO/BaseSocketChannel.swift
Outdated
| promise?.fail(error: error) | ||
| assert(self.isOpen) | ||
| // We're going to set the promise as the pending connect promise, and let close0 fail it for us. | ||
| self.pendingConnect = promise |
There was a problem hiding this comment.
@Lukasa this actually made me aware of another issue (which we may want to handle in another issue tho..). The pendingConnect is currently failed after we call fireChannelInactive which I think is not what should be done as the promise should be notified before the callbacks imho.
Sources/NIO/BaseSocketChannel.swift
Outdated
| @@ -917,7 +917,10 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore { | |||
| becomeActive0(promise: promise) | |||
| } | |||
| } catch let error { | |||
| promise?.fail(error: error) | |||
| assert(self.isOpen) | |||
There was a problem hiding this comment.
in other places we opted to go for assert(self.lifecycleManager.isRegistered). Maybe you also want to assert(!self.lifecycleManager.isActive) because we should be registered but not active here.
There was a problem hiding this comment.
That assertion is unnecessary because the do block covers it. I suppose I could assert isRegistered && !isActive, if you prefer.
Tests/NIOTests/ChannelTests.swift
Outdated
| @@ -83,6 +83,41 @@ class ChannelLifecycleHandler: ChannelInboundHandler { | |||
| } | |||
| } | |||
|
|
|||
| fileprivate class VerifyConnectionFailureHandler: ChannelInboundHandler { | |||
There was a problem hiding this comment.
would you mind putting this class at the bottom of the file? I don't really like when I need to search for the beginning of the actual test class as that is where Xcode puts the 'run all tests in this class' button. I think our test files should all start with the actual test class and not the helpers.
Lukasa
force-pushed
the
cb-sync-connect-failure
branch
3 times, most recently
from
e3c5670
to
57e9460
Compare
|
Ok @weissi, feedback addressed. |
Sources/NIO/BaseSocketChannel.swift
Outdated
| @@ -917,7 +916,10 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore { | |||
| becomeActive0(promise: promise) | |||
| } | |||
| } catch let error { | |||
| promise?.fail(error: error) | |||
| assert(self.lifecycleManager.isRegistered && !self.lifecycleManager.isActive) | |||
There was a problem hiding this comment.
sorry, tiny nit, would you mind putting them in different lines asserts? As if one fails, the assertion message won't tell you what went wrong. Otherwise you'd need to add a message printing both :(
Lukasa
force-pushed
the
cb-sync-connect-failure
branch
from
57e9460
to
eafa908
Compare
Motivation: When a connect() call returns an error other than EINPROGRESS, we currently leave the FD registered and don't close the channel. This is wrong, we should take this as a signal to close the socket immediately, rathern than letting the selector tell us this FD is dead: after all, we know it's dead. Modifications: Call `close0()` in synchronous connect failures. Result: Faster failures!
|
ACK, feedback addressed. |
Lukasa
force-pushed
the
cb-sync-connect-failure
branch
from
eafa908
to
0ad42c3
Compare
Motivation:
When a connect() call returns an error other than EINPROGRESS, we
currently leave the FD registered and don't close the channel. This is
wrong, we should take this as a signal to close the socket immediately,
rathern than letting the selector tell us this FD is dead: after all,
we know it's dead.
Modifications:
Call
close0()in synchronous connect failures.Result:
Faster failures!
Resolves #322.