-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Handle ping correctly in NioTransport #25462
Conversation
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.
left a question
@@ -75,7 +75,11 @@ public int read() throws IOException { | |||
|
|||
try { | |||
BytesReference messageWithoutHeader = message.slice(6, message.length() - 6); | |||
handler.handleMessage(messageWithoutHeader, channel, channel.getProfile(), messageWithoutHeader.length()); | |||
|
|||
// A message length of 6 bytes it is just a ping. Ignore for now. |
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.
right, should we check if the header is correct and if not close the connection?
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 happens in the TcpFrameDecoder
here. If the header is invalid, an exception is thrown .
When the exception is caught, the channel goes through out normal exception handling (and disconnect) logic.
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.
good! just leave a comment about this in the code?
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 added a comment next to the usage of frame decoder to indicate that it would thrown an exception if the message turned out to be invalid.
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.
LGTM.
Merged. Thanks @s1monw and @jasontedor |
* master: (129 commits) Add doc note regarding explicit publish host Fix typo in name of test Add additional test for sequence-number recovery WrapperQueryBuilder should also rewrite the parsed query. Remove dead code and stale Javadoc Update defaults in documentation (#25483) [DOCS] Add docs-dir to Painless (#25482) Add concurrent deprecation logger test [DOCS] Update shared attributes for Elasticsearch (#25479) Use LRU set to reduce repeat deprecation messages Add NioTransport threads to thread name checks (#25477) Add shortcut for AbstractQueryBuilder.parseInnerQueryBuilder to QueryShardContext Prevent channel enqueue after selector close (#25478) Fix Java 9 compilation issue Remove unregistered `transport.netty.*` settings (#25476) Handle ping correctly in NioTransport (#25462) Tests: Remove platform specific assertion in NioSocketChannelTests Remove QueryParseContext from parsing QueryBuilders (#25448) Promote replica on the highest version node (#25277) test: added not null assertion ...
Our current TCPTransport logic assumes that we do not pass pings to
the TCPTransport level.
This commit fixes an issue where NioTransport was passing pings to
TCPTransport and leading to exceptions.