Skip to content
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.

Sign up for GitHub

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

Issue #5794 ensuring serverChannel is closed in the event of a failed bind to ensure proper resources are cleaned #5797

Merged
merged 1 commit into from
Dec 14, 2020
Merged

Issue #5794 ensuring serverChannel is closed in the event of a failed bind to ensure proper resources are cleaned #5797

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions jetty-server/src/main/java/org/eclipse/jetty/server/ServerConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,14 @@ protected ServerSocketChannel openAcceptChannel() throws IOException
}
catch (BindException e)
{
try
{
serverChannel.close();
}
catch (IOException ioe)
{
LOG.warn(ioe);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rework this fix to use try-with-resources, so there is no need of an explicit close?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only want the serverChannel closed if the BindException occurs. Otherwise it needs to be returned to the caller in an open state. I don't believe try-with-resources is what we want here but let me know if I misunderstood.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbordet try-with-resources doesn't work in this case as we use the socket in the successful case... actually probably should null out the field... just in case....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually no fields were set, but there was one already in a try-with-resources, so backed the change out of that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the serverChannel is within the method scope so nothing to null out. I looked at the caller scope or otherwise to see if there was a better place to adjust the close (such as a try with resources). And I'm not seeing that. For the specific issue observed the change as I have it closes that problem. If this is part of a larger concern I'm not sure I follow. Do you need any change from the patch as-is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: using try-with-resources: doh :)

@gregw #5802 is the same fix? @joewitt can you use IO.close() instead? Also, I think we want to catch everything that may possibly be throws after ServerSocketChannel.open(), so I would widen the try scope.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorporating this change within #5802.

throw new IOException("Failed to bind to " + bindAddress, e);
}
}
Expand Down