Allow to free direct buffers on java9 again by normanmaurer · Pull Request #6568 · netty/netty

normanmaurer · 2017-03-25T00:09:29Z

Motivation:

Java9 adds a new method to Unsafe which allows to free direct ByteBuffer via the cleaner without the need to use an commandline arguments.

Modifications:

Add Cleaner interface
Add CleanerJava9 which will be used when using Java9+ and take care of release direct ByteBuffer
Let Cleaner0 implement Cleaner

Result:

Be able to free direct ByteBuffer on Java9+ again without any commandline arguments.

normanmaurer · 2017-03-25T00:09:56Z

common/src/main/java/io/netty/util/internal/PlatformDependent0.java

                DIRECT_BUFFER_CONSTRUCTOR != null ? "available" : "unavailable");
-
-        if (direct != null) {
-            freeDirectBuffer(direct);


removing this makes it easier to break cycle references and its not super important anyway

johnou · 2017-03-25T11:46:45Z

@normanmaurer how about the non unsafe version? https://bugs.openjdk.java.net/browse/JDK-8148117

normanmaurer · 2017-03-26T00:24:45Z

@johnou not sure i get your question... Can you explain ?

normanmaurer · 2017-03-26T00:30:58Z

common/src/main/java/io/netty/util/internal/Cleaner0.java

 * For more details see <a href="https://github.com/netty/netty/issues/2604">#2604</a>.
 */
-final class Cleaner0 {
+final class Cleaner0 implements Cleaner {


I think its fine... @Scottmitch @nmittler @trustin PTAL

It's a possible deadlock such as google/guava#1977?

I think I found the cycle... stay tuned

johnou · 2017-03-26T16:05:05Z

@normanmaurer nvm, I misread "If, at some point in the future, it is determined that sun.misc.Cleaner is in fact a critical internal API ( with static usage ),
then it can be reinstated as a subtype of jdk.internal.ref.Cleaner, providing the same public API. " as though there would already be a public API.

normanmaurer · 2017-03-27T03:33:27Z

@trustin PTAL as well

normanmaurer · 2017-03-28T12:42:16Z

common/src/main/java/io/netty/util/internal/PlatformDependent0.java

                    try {
                        Class<?> bitsClass =
-                                Class.forName("java.nio.Bits", false, PlatformDependent.getSystemClassLoader());
+                                Class.forName("java.nio.Bits", false, getSystemClassLoader());


This fixed a cycle reference

normanmaurer · 2017-03-28T12:42:49Z

common/src/main/java/io/netty/util/internal/PlatformDependent0.java


-        if (direct != null) {
-            freeDirectBuffer(direct);
+    private static boolean explicitNoUnsafe0() {


I moved this from PlatformDependent to remove a cycle reference.

Scottmitch · 2017-03-29T19:08:48Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

+        INVOKE_CLEANER = method;
+    }
+
+    private static void freeDirectBuffer0(ByteBuffer buffer) {


is this method necessary? seems like it is only used in a single place

Scottmitch · 2017-03-29T19:10:35Z

common/src/main/java/io/netty/util/internal/Cleaner0.java

    }

-    static void freeDirectBuffer(ByteBuffer buffer) {
+    private static void freeDirectBuffer0(ByteBuffer buffer) {


I think we can remove this method, and just move the code into freeDirectBuffer. We invoke the cleaner above clean.invoke(cleaner); so we may be double freeing this buffer

Scottmitch · 2017-03-29T19:12:47Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

        }

-        if (!hasUnsafe() && !isAndroid() && !IS_EXPLICIT_NO_UNSAFE) {
+        if (!hasUnsafe() && !isAndroid() && !PlatformDependent0.isExplicitNoUnsafe()) {


isExplicitNoUnsafe should be reflected in hasUnsafe ... can we just remove the !PlatformDependent0.isExplicitNoUnsafe() part?

Scottmitch · 2017-03-29T19:13:45Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

+        if (hasUnsafe() && !isAndroid()) {
+            // only direct to method if we are not running on android.
+            // See https://github.com/netty/netty/issues/2604
+            CLEANER = javaVersion() >= 9 ? new CleanerJava9() : new Cleaner0();


consider renaming Cleaner0 to CleanerJava6.

normanmaurer · 2017-03-29T19:24:52Z

@Scottmitch addressed all of it... Good to merge ?

Scottmitch · 2017-03-29T19:28:49Z

common/src/main/java/io/netty/util/internal/CleanerJava6.java

the conditional below checks if error == null and then says "cleaner is available". However this may not be the case if we have no unsafe. We should adjust the logic below or set the error = <exception indicating unsafe isn't available>.

Scottmitch · 2017-03-29T19:29:58Z

common/src/main/java/io/netty/util/internal/CleanerJava6.java

nit: remove this assert ... we will very likely NPE via a JVM trap below anyways.

Scottmitch · 2017-03-29T19:30:25Z

common/src/main/java/io/netty/util/internal/CleanerJava6.java

can we log a debug statement, or is this expected to happen frequently?

i would argue this should never happen as we tried it before in the static block. Maybe best to throw an Error . WDYT ?

yah if we never expect it to happen lets re-throw it. What about PlatformDependent.throwException(t);?

Scottmitch · 2017-03-29T19:33:34Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

we should also log something similar to the Java6 class if the cleaner is not supported.

Scottmitch · 2017-03-29T19:37:33Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

suggestion: instead of checking this condition every time we could have PlatformDependent detect if the cleaner is properly initialized, and if not just use the NOOP cleaner. We should do the same thing for the java6 class:

class CleanerJava9 { static { .. } static boolean isInitialized() { return INVOKE_CLEANER != null; } .. } class PlatformDependent { .. if (java9) { CLEANER = CleanerJava9.isInitialized() ? new CleanerJava9() : NOOP; } else { CLEANER = CleanerJava6.isInitialized() ? new CleanerJava6() : NOOP; } ... }

Scottmitch

few comments then lgtm

Scottmitch · 2017-03-30T20:11:18Z

common/src/main/java/io/netty/util/internal/CleanerJava6.java


-        // free buffer if possible
-        freeDirectBuffer(direct);
+    public static boolean isSupported() {


package private?

Scottmitch · 2017-03-30T20:12:08Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

+        INVOKE_CLEANER = method;
+    }
+
+    public static boolean isSupported() {


package private?

Scottmitch · 2017-03-30T20:12:27Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

+    private static final Method INVOKE_CLEANER;
+
+    static {
+        Method method;


Scottmitch · 2017-03-30T20:12:31Z

common/src/main/java/io/netty/util/internal/CleanerJava9.java

+
+    static {
+        Method method;
+        Throwable error;


Motivation: Java9 adds a new method to Unsafe which allows to free direct ByteBuffer via the cleaner without the need to use an commandline arguments. Modifications: - Add Cleaner interface - Add CleanerJava9 which will be used when using Java9+ and take care of release direct ByteBuffer - Let Cleaner0 implement Cleaner Result: Be able to free direct ByteBuffer on Java9+ again without any commandline arguments.

normanmaurer · 2017-04-17T17:42:30Z

Cherry-picked into 4.1 (7b6119a) and 4.0 (295c24e)

jasontedor · 2017-05-03T12:34:43Z

common/src/main/java/io/netty/util/internal/PlatformDependent.java

            logger.debug("-Dio.netty.noPreferDirect: {}", !DIRECT_BUFFER_PREFERRED);
        }

-        if (!hasUnsafe() && !isAndroid() && !IS_EXPLICIT_NO_UNSAFE) {


Why was this removed? It means that logging for this shows up even if no unsafe was explicitly set which was the point of adding this in the first place. It means that with Netty 4.1.10.Final our users are back to seeing a message that they don't understand that makes them think their system is broken. Unless there is a good reason that this is a removed (I'm sorry if I'm missing it), I would like to open a PR to add it back.

Relates #5624

Also, it seems to me that this change was not needed for the stated purpose of this change (to allow freeing direct buffers on JDK 9)? Again, maybe I'm missing something and if so I'm sorry.

Adding unrelated changes to PRs makes it hard to follow what is going on. I follow this repository, if I see a change titled "Allow to free direct buffers on java9 again" I think I know what it's about and I skip reading the code change. If an unrelated code change is slipped in, how can I as a follower know this without reading every code change which I think is an unreasonable assumption?

Had this been in a separate PR I would have seen it and would have responded then to please not remove it.

normanmaurer · 2017-05-03T12:39:28Z

There was no good reason and it's my fault to miss this :( Can you please do a PR and also add a comment in the code why it should never be removed again. Sorry again for the trouble

…

Am 03.05.2017 um 05:36 schrieb Jason Tedor ***@***.***>: @jasontedor commented on this pull request. In common/src/main/java/io/netty/util/internal/PlatformDependent.java: > @@ -122,7 +127,7 @@ public Random current() { logger.debug("-Dio.netty.noPreferDirect: {}", !DIRECT_BUFFER_PREFERRED); } - if (!hasUnsafe() && !isAndroid() && !IS_EXPLICIT_NO_UNSAFE) { Relates #5624 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

jasontedor · 2017-05-03T13:20:44Z

Thank you for understanding @normanmaurer! I will open a PR soon.

Can you please do a PR and also add a comment in the code why it should never be removed again.

I will do this! 😄

jasontedor · 2017-05-03T16:56:44Z

I opened #6696.

normanmaurer commented Mar 25, 2017

View reviewed changes

normanmaurer requested review from Scottmitch, nmittler and trustin March 25, 2017 00:10

normanmaurer self-assigned this Mar 25, 2017

normanmaurer added the defect label Mar 25, 2017

normanmaurer added this to the 4.0.46.Final milestone Mar 25, 2017

normanmaurer commented Mar 26, 2017

View reviewed changes

normanmaurer force-pushed the cleaner9 branch 2 times, most recently from cb58839 to 4fde8fa Compare March 28, 2017 08:16

normanmaurer commented Mar 28, 2017

View reviewed changes

Scottmitch requested changes Mar 29, 2017

View reviewed changes

normanmaurer force-pushed the cleaner9 branch from 4fde8fa to a4a1839 Compare March 29, 2017 19:24

normanmaurer force-pushed the cleaner9 branch from a4a1839 to 4ce9f4f Compare March 29, 2017 19:25

Scottmitch requested changes Mar 29, 2017

View reviewed changes

normanmaurer force-pushed the cleaner9 branch 2 times, most recently from 580c10b to 8de8345 Compare March 29, 2017 20:23

Scottmitch requested changes Mar 30, 2017

View reviewed changes

normanmaurer force-pushed the cleaner9 branch from 8de8345 to f209cae Compare March 31, 2017 06:33

normanmaurer closed this Apr 17, 2017

normanmaurer deleted the cleaner9 branch April 17, 2017 17:42

jasontedor reviewed May 3, 2017

View reviewed changes

This was referenced May 3, 2017

You had one job Netty logging guard elastic/elasticsearch#24469

Merged

Do not log on explicit no unsafe again #6696

Closed

jasontedor mentioned this pull request May 12, 2017

Remove Netty logging hack elastic/elasticsearch#24653

Merged

Uh oh!

Conversation

normanmaurer commented Mar 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnou commented Mar 25, 2017

Uh oh!

normanmaurer commented Mar 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fenik17 Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnou commented Mar 26, 2017

Uh oh!

normanmaurer commented Mar 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Scottmitch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented Apr 17, 2017

Uh oh!

jasontedor May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer commented May 3, 2017 via email

Uh oh!

jasontedor commented May 3, 2017

Uh oh!

jasontedor commented May 3, 2017

fenik17 Mar 27, 2017 •

edited

Loading

normanmaurer Mar 28, 2017 •

edited

Loading

jasontedor May 3, 2017 •

edited

Loading