-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 discovery exception handling #1160
Conversation
I think you should disconnect it in case of error anyway, should you? |
UDP is connectionless protocol, thus there is no session that can be closed. |
Thanks a lot to @JMdoubleU for pointing to that problem! |
try { | ||
nodeManager.handleInbound(event); | ||
} catch (Throwable t) { | ||
logger.error("Failed to process incoming message: {}", event.getMessage(), t); |
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 would change log level to WARN
or even INFO
, since we can receive any garbage and this is not our error.
I'd also output t.toString()
instead of t
to not flood logs with traces in case of invalid messages
Ideally we can also try to differentiate the message handle problems onto MalformedMessageException
and others. The first one is not error, the second one should be assumed as our 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.
Decided to reduce logging level for the beginning. MalformedMessageException
requires much more changes in the codebase.
oops, forgot that discovery works on UDP |
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 good to me
Fixes
UDPListener
crash caused by malformednodeId
received from malicious peers.