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 Oct 25, 2019
@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 @execution from:member labels Oct 25, 2019
@lptr lptr added this to the 5.6.4 milestone Oct 25, 2019
Copy link
Member

@wolfs wolfs left a comment

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

@wolfs wolfs Oct 25, 2019

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

@vRallev vRallev Oct 25, 2019

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

@lptr lptr Oct 25, 2019

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

@wolfs wolfs Oct 25, 2019

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

@wolfs wolfs Oct 25, 2019

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

@lptr lptr Oct 25, 2019

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 Oct 28, 2019
@lptr lptr requested a review from wolfs Oct 28, 2019
@lptr
Copy link
Member Author

@lptr 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

@wolfs wolfs Oct 28, 2019

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

@lptr lptr Oct 28, 2019

Choose a reason for hiding this comment

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

It's down there :)

wolfs
wolfs approved these changes Oct 28, 2019
Copy link
Member

@wolfs wolfs left a comment

LGTM!

@lptr lptr merged commit faa7e45 into release-5.6 Oct 28, 2019
5 checks passed
@lptr lptr deleted the lptr/network/fix-localhost-lookup-bug branch Oct 28, 2019
@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 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 big-guy commented Nov 5, 2019

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

@lptr
Copy link
Member Author

@lptr 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
@execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants