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

Use InetAddressResolverProvider and add tests #1353

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale requested a review from jyemin March 27, 2024 00:02
@stIncMale stIncMale self-assigned this Mar 27, 2024
Comment on lines -189 to -190
* <p>If set, it will be used to look up the {@link java.net.InetAddress} for each host, via
* {@link InetAddressResolver#lookupByName(String)}. Otherwise, {@link java.net.InetAddress#getAllByName(String)} will be used.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was incorrect, as "Otherwise, ..." did not take into account InetAddressResolverProvider. Builder#inetAddressResolver explains the behavior in full, so I added @see Builder#inetAddressResolver(InetAddressResolver).

com.mongodb.UnixServerAddress,\
com.mongodb.internal.connection.SnappyCompressor,\
com.mongodb.internal.connection.ClientMetadataHelper,\
com.mongodb.internal.connection.ServerAddressHelper
Copy link
Member Author

Choose a reason for hiding this comment

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

I checked by looking at the reports generated by native-image that these \ line breaks work fine.

{
"resources":{
"includes":[{
"pattern":"\\QMETA-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider\\E"
Copy link
Member Author

Choose a reason for hiding this comment

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

Weirdly, everything works even without this entry, but, if I collect reachability metadata, then this entry is added there.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange. Did you find that for any other reachability metatada? Can you suggest a hypothesis for what might be going on?

Copy link
Member Author

@stIncMale stIncMale Mar 28, 2024

Choose a reason for hiding this comment

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

Did you find that for any other reachability metatada?

I haven't checked everything, nor do I understand how some of the automatically collected resources are used, but removing the shared library resources, or the logback-test.xml resource do have an effect: the former caused runtime errors, the latter results in log entries disappearing.

Can you suggest a hypothesis for what might be going on?

I suspect, it's all about undocumented behavior that is intended to make things easier. I found the following:

Of the two experimental options mentioned in the PR comment linked above, only -H:-UseServiceLoaderFeature is recognized, and it breaks ServiceLoader completely:

  • if we don't have the META-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider resource entry, ServiceLoader silently fails to locate CustomInetAddressResolverProvider and CustomInetAddressResolverProvider.assertUsed() fails
  • if we do have the META-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider resource entry, the ServiceLoader fails with java.util.ServiceConfigurationError: com.mongodb.spi.dns.InetAddressResolverProvider: Provider com.mongodb.internal.graalvm.CustomInetAddressResolverProvider not found even if I re-collect the reachability metadata (it does not find anything new, but it should have registered CustomInetAddressResolverProvider for reflection via reflect-config.json).

P.S. Before I found the above, I did and wrote the following:

I had a uneducated guess that maybe the entry is not needed because the META-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider resource is in the same project as the one I am building, and if the resource were in a pre-built JAR that I depend on, then the resource entry would have been required.

Since you asked, I tested that guess: created a new Gradle module with the resource and CustomInetAddressResolverProvider, removed those from :graalvm-native-image-app, and added the configuration:'archives' dependency on the new module to :graalvm-native-image-app. The result is: the entry about META-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider in resource-config.json is still not needed. Note that when there is no entry, the resource is actually not mentioned in the registerResource messages emitted by native-image when it builds, yet CustomInetAddressResolverProvider is still successfully located via ServiceLoader.

I don't have any other hypotheses. The behavior is really weird.

// informing us on the kind of initialization for each Java class
buildArgs.add('-H:+PrintClassInitialization')
// see class initialization and other reports in `graalvm/build/native/nativeCompile/reports`
buildArgs.add('--diagnostics-mode')
Copy link
Member Author

Choose a reason for hiding this comment

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

--diagnostics-mode includes the functionality of -H:+PrintClassInitialization, and more. So we better use --diagnostics-mode.


import java.util.ArrayList;

final class DnsSpi {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we also have com.mongodb.spi.dns.DnsClientProvider, consider renaming this to InetAddressResolverSpi.

Copy link
Member Author

@stIncMale stIncMale Mar 28, 2024

Choose a reason for hiding this comment

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

My plan is to use a custom DnsClientProvider in DnsSpi as part of JAVA-5219, and assert it is used similarly to what I've done in the current PR. Which is why the current class name seems appropriate to me. What do you think?

{
"resources":{
"includes":[{
"pattern":"\\QMETA-INF/services/com.mongodb.spi.dns.InetAddressResolverProvider\\E"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange. Did you find that for any other reachability metatada? Can you suggest a hypothesis for what might be going on?

@stIncMale stIncMale requested a review from jyemin March 28, 2024 01:45
@stIncMale stIncMale merged commit 9a02f81 into mongodb:master Mar 28, 2024
60 checks passed
@stIncMale stIncMale deleted the JAVA-5390 branch March 28, 2024 13:20
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.

2 participants