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

Propagate all exceptions when loading native code #7250

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@carl-mastrangelo
Member

carl-mastrangelo commented Sep 26, 2017

Motivation:
There are 2 motivations, the first depends on the second:

Loading Netty Epoll statically stopped working in 4.1.16, due to
Native always loading the arch specific shared object. In a
static binary, there is no arch specific SO.

Second, there are a ton of exceptions that can happen when loading
a native library. When loading native code, Netty tries a bunch of
different paths but a failure in any given may not be fatal.

Additionally: turning on debug logging is not always feasible so
exceptions get silently swallowed.

Modifications:

  • Change Epoll and Kqueue to try the static load second
  • Modify NativeLibraryLoader to record all the locations where
    exceptions occur.
  • Attempt to use addSuppressed from Java 7 if available.

Alternatives Considered:

An alternative would be to record log messages at each failure. If
all load attempts fail, the log messages are printed as warning,
else as debug. The problem with this is there is no LogRecord to
create like in java.util.logging. Buffering the args to
logger.log() at the end of the method loses the call site, and
changes the order of events to be confusing.

Another alternative is to teach NativeLibraryLoader about loading
the SO first, and then the static version. This would consolidate
the code fore Epoll, Kqueue, and TCNative. I think this is the
long term better option, but this PR is changing a lot already.
Someone else can take a crack at it later

Results:
Epoll Still Loads and easier debugging.

Fixes #7249

Propagate all exceptions when loading native code
Motivation:
There are 2 motivations, the first depends on the second:

Loading Netty Epoll statically stopped working in 4.1.16, due to
`Native` always loading the arch specific shared object.  In a
static binary, there is no arch specific SO.

Second, there are a ton of exceptions that can happen when loading
a native library.  When loading native code, Netty tries a bunch of
different paths but a failure in any given may not be fatal.

Additionally: turning on debug logging is not always feasible so
exceptions get silently swallowed.

Modifications:

* Change Epoll and Kqueue to try the static load second
* Modify NativeLibraryLoader to record all the locations where
  exceptions occur.
* Attempt to use `addSuppressed` from Java 7 if available.

Alternatives Considered:

An alternative would be to record log messages at each failure.  If
all load attempts fail, the log messages are printed as warning,
else as debug. The problem with this is there is no `LogRecord` to
create like in java.util.logging.  Buffering the args to
logger.log() at the end of the method loses the call site, and
changes the order of events to be confusing.

Another alternative is to teach NativeLibraryLoader about loading
the SO first, and then the static version.  This would consolidate
the code fore Epoll, Kqueue, and TCNative.   I think this is the
long term better option, but this PR is changing a lot already.
Someone else can take a crack at it later

Results:
Epoll Still Loads and easier debugging.
try {
// Define the helper class in the target ClassLoader,
// then we can call the helper to load the native library.
Method defineClass = ClassLoader.class.getDeclaredMethod("defineClass", String.class,

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Sep 26, 2017

Member

Also, this looks like a possible bug. Shouldn't loader be used to lookup defineClass, not ClassLoader.class ?

cc: @ejona86

@carl-mastrangelo

carl-mastrangelo Sep 26, 2017

Member

Also, this looks like a possible bug. Shouldn't loader be used to lookup defineClass, not ClassLoader.class ?

cc: @ejona86

This comment has been minimized.

@normanmaurer

normanmaurer Sep 26, 2017

Member

good question...

@normanmaurer

normanmaurer Sep 26, 2017

Member

good question...

This comment has been minimized.

@ejona86

ejona86 Sep 26, 2017

Contributor

Since loader is known to be an instanceof ClassLoader using ClassLoader.class here is fine. It stands out as strange because the reflection is to avoid accessibility restrictions, not because the class is unknown at compile time. So it's good and fine as-is, just most other reflection usages couldn't do this.

@ejona86

ejona86 Sep 26, 2017

Contributor

Since loader is known to be an instanceof ClassLoader using ClassLoader.class here is fine. It stands out as strange because the reflection is to avoid accessibility restrictions, not because the class is unknown at compile time. So it's good and fine as-is, just most other reflection usages couldn't do this.

return AccessController.doPrivileged(new PrivilegedAction<Class<?>>() {
@Override
public Class<?> run() {
try {

This comment has been minimized.

@netkins

netkins Sep 26, 2017

MAJOR Extract this nested try block into a separate method. rule

@netkins

netkins Sep 26, 2017

MAJOR Extract this nested try block into a separate method. rule

// Make sure the helper is belong to the target ClassLoader.
final Class<?> newHelper = tryToLoadClass(loader, NativeLibraryUtil.class);
loadLibraryByHelper(newHelper, name, absolute);
try {

This comment has been minimized.

@netkins

netkins Sep 26, 2017

MAJOR Extract this nested try block into a separate method. rule

@netkins

netkins Sep 26, 2017

MAJOR Extract this nested try block into a separate method. rule

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 26, 2017

Member

@Scottmitch PTAL as well.

Member

normanmaurer commented Sep 26, 2017

@Scottmitch PTAL as well.

@normanmaurer normanmaurer self-assigned this Sep 26, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 4, 2017

Member

Cherry-picked into 4.1 (d3ca087) and 4.0 (4a572e0)

Member

normanmaurer commented Oct 4, 2017

Cherry-picked into 4.1 (d3ca087) and 4.0 (4a572e0)

@normanmaurer normanmaurer added the defect label Oct 4, 2017

@normanmaurer normanmaurer added this to the 4.0.53.Final milestone Oct 4, 2017

@carl-mastrangelo carl-mastrangelo deleted the carl-mastrangelo:nettynative branch Oct 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment