HADOOP-18024. SocketChannel is not closed when IOException happens in Server$Listener.doAccept #3719
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
This is a follow-up for #2727. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
reviewed
Nov 29, 2021
| try { | ||
| channel.socket().close(); | ||
| channel.close(); | ||
| } catch (IOException ignored) { |
Contributor
There was a problem hiding this comment.
Good to log Exception here as Channel could not be closed
Contributor
Author
There was a problem hiding this comment.
I've added the log in a new commit. Thanks
|
💔 -1 overall
This message was automatically generated. |
virajjasani
reviewed
Dec 3, 2021
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Changes look good, any chance we could add unit test?
Contributor
Author
|
@virajjasani I've pushed a commit for the unit test Then I just override some methods of TestServer and Server$Listener to inject the IOException.In testIOEOnListenerAccept, the first client call encounters the injection in server and thus gets its socket closed. Hence we expect EOFException. The second client call should work normally.Any comment or suggestion? Thank you! |
|
🎊 +1 overall
This message was automatically generated. |
virajjasani
reviewed
Dec 4, 2021
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Left few comments. Thanks for including the test @functioner. Could you please also take care of checkstyle warnings?
|
|
||
| /** Listens on the socket. Creates jobs for the handler threads*/ | ||
| private class Listener extends Thread { | ||
| protected class Listener extends Thread { |
Contributor
There was a problem hiding this comment.
Would have been better if we could keep this private, but it seems we have no better way if we want to test this change (Thanks for the test btw). If we are going to go forward with this, we should annotate this as @VisibleForTesting and @InterfaceAudience.Private.
Let's also get some expert advice on this. cc @aajisaka @ayushtkn @jojochuang
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
@virajjasani Thank you for pointing out the checkstyle issue. I've pushed a new commit to resolve it. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
This time, |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
|
Seems better. Thanks @functioner. Once more checkstyle warning is pending related to Javadoc. |
Contributor
Author
|
@virajjasani Sorry that I overlooked this. I've just pushed a new commit. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Are there any other issues or concern with the current patch and test? If any, I will resolve ASAP. Will this PR be merged soon by any chance? |
Contributor
|
+1 (non-binding), Thanks @functioner |
iwasakims
approved these changes
Dec 8, 2021
Member
iwasakims
left a comment
iwasakims
left a comment
There was a problem hiding this comment.
LGTM. mvn test -Dtest=TestIPC#testIOEOnListenerAccept timed out as expected if I commented out the part closing channel in Server.java.
Member
|
I merged this. Thanks, @functioner and @virajjasani. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
… Server$Listener.doAccept (apache#3719)
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ppens in Server$Listener.doAccept (apache#3719)" This reverts commit 6ed0158. Breaks TestIPC#testIOEOnListenerAccept
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
HADOOP-18024
How was this patch tested?
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?