-
Notifications
You must be signed in to change notification settings - Fork 47
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
Initialize state machine on handlerAdded #43
Conversation
Can one of the admins verify this patch? |
3 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
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.
One minor note about the implementation but it otherwise looks good. I'd love to see a test as well here.
Sources/NIOSSH/NIOSSHHandler.swift
Outdated
@@ -48,6 +48,9 @@ public final class NIOSSHHandler { | |||
// Whether we're expecting a channelReadComplete. | |||
private var expectingChannelReadComplete: Bool = false | |||
|
|||
// Whether the state machine execution started. | |||
private var initialized: Bool = false |
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 don't think we need this: we can just remove the crash from SSHConnectionStateMachine.start()
and have that method return nil
instead.
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 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, approach looks good. If we get our tests we're good to go.
Motivation: Sometimes we already have an established channel, for example, one returned by establishing a tunneled connection. If this is the case, then we cannot use it to start key exchange, since it's only started when channel becomes active. In order to support more use cases we should handle starting key exchange when handler is added as well. Modifications: 1. Extract key exchange initialization to separate function 2. Call it from handlerAdded if channel is active 3. Guard againts calling it twice Result: Fixes #42
@Lukasa done, btw, could you whitelist me so I can start tests? |
@swift-server-bot add to whitelist |
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.
Great, looks good!
Motivation:
Sometimes we already have an established channel, for example,
one returned by establishing a tunneled connection. If this
is the case, then we cannot use it to start key exchange, since
it's only started when channel becomes active. In order to
support more use cases we should handle starting key exchange
when handler is added as well.
Modifications:
Result:
Fixes #42