-
Notifications
You must be signed in to change notification settings - Fork 378
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
Fix on handshake failure not disconnecting connection #6179
Fix on handshake failure not disconnecting connection #6179
Conversation
string clientId = $"unknown {_session?.RemoteHost}"; | ||
try | ||
{ | ||
clientId = _session?.Node?.ToString(Node.Format.Console); |
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 does this throw exceptions? Can we have a version that is guaranteed not to throw?
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.
The Node
property throw exception because RemoteId
is still missing at this point. Changing that to a nullable might make other place more inconviniecne.
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.
Can we handle that in ToString
method and not throw exceptions?
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.
Which ToString
? The Node
property of the _session
that is throwing.
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.
Cosmetic: instead of having empty catch we could move the below checks to session bool property:
if (_node is null)
{
if (RemoteNodeId is null || RemoteHost is null || RemotePort == 0)
{
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.
Ah can we introduce Session.TryGetNode?
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 just add if (_session.RemoteNodeId != null)
in front of it.
@@ -127,7 +135,7 @@ public override void ExceptionCaught(IChannelHandlerContext context, Exception e | |||
} | |||
} | |||
|
|||
base.ExceptionCaught(context, exception); | |||
_ = context.DisconnectAsync(); |
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.
Don't we need to call base class anymore? Why?
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.
Calling base class does nothing. It just pass the exception away to the tail.
string clientId = $"unknown {_session?.RemoteHost}"; | ||
try | ||
{ | ||
clientId = _session?.Node?.ToString(Node.Format.Console); |
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.
Cosmetic: instead of having empty catch we could move the below checks to session bool property:
if (_node is null)
{
if (RemoteNodeId is null || RemoteHost is null || RemotePort == 0)
{
* Fix on handshake failure not disconnecting * Addressing comment
* Fix on handshake failure not disconnecting * Addressing comment
Changes
ExceptionCaught
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing