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

Try to load native linux libraries with matching classifier first #9411

Merged
merged 1 commit into from Aug 12, 2019

Conversation

NicoK
Copy link
Contributor

@NicoK NicoK commented Jul 29, 2019

Motivation:

Users' runtime systems may have incompatible dynamic libraries to the ones our
tcnative wrappers link to. Unfortunately, we cannot determine and catch these
scenarios (in which the JVM crashes) but we can make a more educated guess on
what library to load and try to find one that works better before crashing.

Modifications:

Based on #9410 and netty/netty-tcnative#480, this PR adds:

  • Load native linux libraries with matching classifier (first) and only afterwards try the generic one

Result:

More developers / users can use the dynamically-linked native libraries.


This change is Reviewable

@netty-bot
Copy link

Can one of the admins verify this patch?

@normanmaurer
Copy link
Member

@netty-bot test this please

Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

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

Please address comments and also rebase

return Collections.unmodifiableSet(LINUX_OS_CLASSIFIERS);
}

private static void addClassifier(HashSet<String> allowedClassifiers, String... split) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Set

final String[] OS_RELEASE_FILES = {"/etc/os-release", "/usr/lib/os-release"};
final String LINUX_ID_PREFIX = "ID=";
final String LINUX_ID_LIKE_PREFIX = "ID_LIKE=";
final HashSet<String> allowedClassifiers = new HashSet<String>();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Set

@@ -1274,6 +1329,23 @@ public static String normalizedOs() {
return NORMALIZED_OS;
}

public static Set<String> normalizedLinuxClassifiers() {
return Collections.unmodifiableSet(LINUX_OS_CLASSIFIERS);
Copy link
Member

Choose a reason for hiding this comment

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

Why not create this once in the static method and return the same instance all the time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea - I'll do it in the static initializer though

@@ -97,6 +106,10 @@
private static final String NORMALIZED_ARCH = normalizeArch(SystemPropertyUtil.get("os.arch", ""));
private static final String NORMALIZED_OS = normalizeOs(SystemPropertyUtil.get("os.name", ""));

// keep in sync with maven builds!
Copy link
Member

Choose a reason for hiding this comment

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

Maybe also add a comment that this is defined in pom.xml ?

Motivation:

Users' runtime systems may have incompatible dynamic libraries to the ones our
tcnative wrappers link to. Unfortunately, we cannot determine and catch these
scenarios (in which the JVM crashes) but we can make a more educated guess on
what library to load and try to find one that works better before crashing.

Modifications:

1) Build dynamically linked openSSL builds for more OSs (netty-tcnative)
2) Load native linux libraries with matching classifier (first)

Result:

More developers / users can use the dynamically-linked native libraries.
@NicoK
Copy link
Contributor Author

NicoK commented Aug 7, 2019

thanks for the review, I believe, I have addressed your comments now

@NicoK
Copy link
Contributor Author

NicoK commented Aug 7, 2019

@normanmaurer can you give me a hint for what the next steps are? We probably need to create new docker files for the new distributions but is that enough then to get release artifacts?

@normanmaurer
Copy link
Member

@netty-bot test this please

@normanmaurer
Copy link
Member

@NicoK I need to think about how to process after this to make sure we also release extra native artefacts.

@normanmaurer normanmaurer merged commit 8d9cea2 into netty:4.1 Aug 12, 2019
normanmaurer pushed a commit that referenced this pull request Aug 12, 2019
)

Motivation:

Users' runtime systems may have incompatible dynamic libraries to the ones our
tcnative wrappers link to. Unfortunately, we cannot determine and catch these
scenarios (in which the JVM crashes) but we can make a more educated guess on
what library to load and try to find one that works better before crashing.

Modifications:

1) Build dynamically linked openSSL builds for more OSs (netty-tcnative)
2) Load native linux libraries with matching classifier (first)

Result:

More developers / users can use the dynamically-linked native libraries.
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

3 participants