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
Conversation
There was a problem hiding this 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.
subprojects/base-services/src/main/java/org/gradle/internal/net/InetAddresses.java
Outdated
Show resolved
Hide resolved
// 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This is to work around a five second lag when using the JDK approach, see https://bugs.openjdk.java.net/browse/JDK-8143378.
5b7748a
to
ea7879e
Compare
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's down there :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@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? |
@lptr please let me know if this affects 6.0RC3 at all. |
@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. |
See https://bugs.openjdk.java.net/browse/JDK-8143378
Fixes #11122