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

Support running Netty (in particular netty-tcnative) in the bootstrap class loader #7345

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@trask
Contributor

trask commented Oct 26, 2017

Motivation:

Fix NullPointerExceptions that occur when running netty-tcnative inside the bootstrap class loader.

Modifications:

Replace loader.getResource(...) with ClassLoader.getSystemResource(...) when loader is null.

Replace loader.loadClass(...) with Class.forName(..., false, loader) which works when loader is both null and non-null.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 27, 2017

Member

@trask I wonder if we could somehow have an unit test.

Member

normanmaurer commented Oct 27, 2017

@trask I wonder if we could somehow have an unit test.

@normanmaurer normanmaurer self-assigned this Oct 27, 2017

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Oct 27, 2017

Contributor

For the test, you could add a binary to the bootstrap class loader, but just as a weird name "test-bs-loading.so" and load it in the test. But that'd be pretty annoying.

Contributor

ejona86 commented Oct 27, 2017

For the test, you could add a binary to the bootstrap class loader, but just as a weird name "test-bs-loading.so" and load it in the test. But that'd be pretty annoying.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 27, 2017

Member

@ejona86 @trask if its too hard to add a test than dont worry... I just would prefer to have a unit test so we ensure we not break it again in the future.

@trask let me know what you think.

Member

normanmaurer commented Oct 27, 2017

@ejona86 @trask if its too hard to add a test than dont worry... I just would prefer to have a unit test so we ensure we not break it again in the future.

@trask let me know what you think.

@trask

This comment has been minimized.

Show comment
Hide comment
@trask

trask Oct 27, 2017

Contributor

@normanmaurer @ejona86 unit test added, let me know what you think, the test caught one additional NPE, see additional change in latest push:

if (loader == null) {
    // cannot defineClass inside bootstrap class loader
    throw e1;
}
Contributor

trask commented Oct 27, 2017

@normanmaurer @ejona86 unit test added, let me know what you think, the test caught one additional NPE, see additional change in latest push:

if (loader == null) {
    // cannot defineClass inside bootstrap class loader
    throw e1;
}
@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Oct 27, 2017

Contributor

Oh, hey! Much easier test. Right! The file doesn't even need to exist.

Contributor

ejona86 commented Oct 27, 2017

Oh, hey! Much easier test. Right! The file doesn't even need to exist.

@johnou

johnou approved these changes Oct 28, 2017

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 29, 2017

Member

Cherry-picked into 4.1 (58e74e9) and 4.0 (b12a1e3).

@trask thanks!

Member

normanmaurer commented Oct 29, 2017

Cherry-picked into 4.1 (58e74e9) and 4.0 (b12a1e3).

@trask thanks!

@normanmaurer normanmaurer added the defect label Oct 29, 2017

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

@trask

This comment has been minimized.

Show comment
Hide comment
@trask

trask Oct 29, 2017

Contributor

Thanks for the quick review and merge

Contributor

trask commented Oct 29, 2017

Thanks for the quick review and merge

@@ -25,13 +26,56 @@
public class NativeLibraryLoaderTest {
private static final Method getSupressedMethod = getGetSuppressed();

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Oct 30, 2017

Member

ThrowableUtil might have been a better place for this.

@carl-mastrangelo

carl-mastrangelo Oct 30, 2017

Member

ThrowableUtil might have been a better place for this.

This comment has been minimized.

@trask

trask Oct 30, 2017

Contributor

I thought it would be better not to put test-only code (getSuppressed) inside of non- test code (ThrowableUtil), but if that's not a concern I can move it there.

@trask

trask Oct 30, 2017

Contributor

I thought it would be better not to put test-only code (getSuppressed) inside of non- test code (ThrowableUtil), but if that's not a concern I can move it there.

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