HADOOP-19881. Fix "port in use" detection in TestFrameDecoder#8472
Merged
pan3793 merged 2 commits intoapache:trunkfrom May 8, 2026
Merged
HADOOP-19881. Fix "port in use" detection in TestFrameDecoder#8472pan3793 merged 2 commits intoapache:trunkfrom
pan3793 merged 2 commits intoapache:trunkfrom
Conversation
|
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Pull request overview
This PR updates the TestFrameDecoder test helper that starts a Netty-based RPC server, aiming to make “port already in use” retries reliable and to avoid incorrectly treating thread interrupts as retriable bind failures.
Changes:
- Catch broader bind failures during server startup and retry only when the failure looks like a port-in-use condition.
- Ensure each retry actually changes the port (
+ 1 + rand.nextInt(20)), avoiding a zero increment. - Handle
InterruptedExceptionseparately by restoring the interrupt flag and rethrowing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import java.io.IOException; |
Comment on lines
+266
to
+274
| * bound by another process. Handles {@link ChannelException} thrown by | ||
| * Netty as well as {@link BindException} that may be sneaky-thrown from | ||
| * {@code ChannelFuture#sync()}. | ||
| */ | ||
| private static boolean isPortInUse(Throwable t) { | ||
| Throwable cursor = t; | ||
| while (cursor != null) { | ||
| if (cursor instanceof BindException || cursor instanceof ChannelException) { | ||
| return true; |
Member
Author
There was a problem hiding this comment.
partially adopted. only match BindException, but do not check the message - this is not something in api contract, it might change in future JDK releases.
|
🎊 +1 overall
This message was automatically generated. |
Member
Author
|
@slfan1989 I addressed the Copilot review comments, and Yetus says good. Do you have further comments? |
Contributor
|
LGTM |
slfan1989
approved these changes
May 8, 2026
pan3793
added a commit
that referenced
this pull request
May 8, 2026
Reviewed-by: Shilun Fan <slfan1989@apache.org> Signed-off-by: Cheng Pan <chengpan@apache.org>
Member
Author
|
thanks, merged to trunk/branch-3.5 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
BindExceptionwas not caught — main cause of the failure shown in the log.Netty's
ChannelFuture#sync()usesPlatformDependent.throwExceptionto "sneaky-throw" the original cause. When the OS returns "Address already in use", the cause isjava.net.BindException(which extendsIOException), notChannelException. The originalcatch (InterruptedException | ChannelException e)missed it entirely, so the exception propagated out of the loop and failed the test on the first attempt - exactly matching the stack trace in the failure log.The port increment could be zero.
serverPort += rand.nextInt(20)returns[0, 20), so on retry the same busy port could be picked again. Changed to1 + rand.nextInt(20)so the port is always bumped.InterruptedExceptionwas lumped with "port in use".An external thread interrupt should not trigger a port-bump retry. Split into its own handler that restores the interrupt flag and propagates.
Contains content generated by: Claude Opus 4.7
How was this patch tested?
Run dozens of rounds
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?AI Tooling
If an AI tool was used:
where is the name of the AI tool used.
https://www.apache.org/legal/generative-tooling.html