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.

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

Workaround for slow localhost lookup JDK bug on macOS #11134

Merged
merged 2 commits into from Oct 28, 2019

Conversation

lptr
Copy link
Member

@lptr lptr commented Oct 25, 2019

@lptr lptr requested a review from wolfs October 25, 2019 00:26
@lptr lptr changed the title Fix localhost lookup bug Workaround for localhost lookup bug Oct 25, 2019
@lptr lptr changed the title Workaround for localhost lookup bug Workaround for localhost lookup JDK bug Oct 25, 2019
@lptr lptr self-assigned this Oct 25, 2019
@lptr lptr added this to the 5.6.4 milestone Oct 25, 2019
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

I really wonder why the "usual" workaround doesn't work in this case.

// Work around https://bugs.openjdk.java.net/browse/JDK-8143378 on macOS
// See also https://stackoverflow.com/a/39698914
if (hostName == null && OperatingSystem.current() == OperatingSystem.MAC_OS) {
ProcessBuilder builder = new ProcessBuilder("hostname");
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this should go into native platform instead of forking a process here. I also don't understand why the usual workaround of adding things to hosts doesn't fix the problem any more.

Copy link

Choose a reason for hiding this comment

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

It does, but could have other side effects (I don't know why, but I was getting exceptions in IntelliJ and there Git integration after changing the hosts file). The main problem is that people aren't aware of this issue and simply complain that Gradle is slow. It's not clear why packaging the build cache entries takes 5 seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a lot better to not have the problem than to explain what people need to do to work around it. BTW, the build scan plugin suffers from the same problem, and will be able to use the same (built-in) workaround. (/cc @ldaley)

Copy link
Member

Choose a reason for hiding this comment

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

I fully agree that that it would be better to have everything work out of the box.

import java.util.List;

public interface InetAddressFactory {
String getHostname();
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to extract a simpler service, which only provides the host name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's probably a good idea.

@lptr lptr force-pushed the lptr/network/fix-localhost-lookup-bug branch from 5b7748a to ea7879e Compare October 28, 2019 08:16
@lptr lptr requested a review from wolfs October 28, 2019 08:18
@lptr
Copy link
Member Author

lptr commented Oct 28, 2019

@wolfs I've simplified the fix to be much more limited in scope. Please take another look.

String operatingSystem,
String currentBuildInvocationId,
PropertiesConfigurator additionalProperties,
HostnameLookup hostnameLookup
Copy link
Member

Choose a reason for hiding this comment

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

I can't find the class HostnameLookup. Is that missing from the branch?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's down there :)

Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

LGTM!

@lptr lptr merged commit faa7e45 into release-5.6 Oct 28, 2019
@lptr lptr deleted the lptr/network/fix-localhost-lookup-bug branch October 28, 2019 10:32
@big-guy big-guy changed the title Workaround for localhost lookup JDK bug Workaround for slow localhost lookup JDK bug on macOS Nov 1, 2019
@ldaley
Copy link
Member

ldaley commented Nov 5, 2019

@lptr did you consider that this is actually a different value? This now uses the local hostname on Mac OS, but the public host name on other environments. What does Gradle use this value for?

@big-guy
Copy link
Member

big-guy commented Nov 5, 2019

@lptr please let me know if this affects 6.0RC3 at all.

@lptr
Copy link
Member Author

lptr commented Nov 5, 2019

@ldaley this host name is only used in the origin manifest of cached artifacts. We put it there some time ago hoping it would be of use. I don't know about a use case for it now, so I'm not sure if using the local or the public hostname makes a difference. If we find a use case for it that will clarify this question too.

We could also remove the hostname from the artifact's origin manifest, but I wouldn't want to do that now that we have a fix.

@big-guy we are good to go ahead with 6.0-rc-3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants