IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh… #4063

alamar · 2018-05-24T16:42:20Z

…aller, avoid deadlock.

slukyano · 2018-05-24T19:07:26Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+                            b1.await();
+
+                            if (ii % 2 == 0)
+                                ign.configuration().getMarshaller().unmarshal(one, Thread.currentThread().getContextClassLoader());


Is there a reason to use Thread.currentThread().getContextClassLoader() here?
Generally, you're not supposed to use thread's context class loader unless you set it. It is useful when your code is loaded by one class loader but it can't load the target class you want to use.
When you want to download a class with "whatever the current class loader is", use this.getClass().getClassLoader().

I'll try without it.

slukyano · 2018-05-24T19:10:39Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+            final CyclicBarrier b1 = new CyclicBarrier(4);
+            final CyclicBarrier b2 = new CyclicBarrier(5);
+
+            for (int i = 0; i < 4; i++) {


I'd went with just two threads here - enough to reproduce the issue (and reproduce it quite often, right?) without additional complications.

I'll try that.

slukyano · 2018-05-24T19:17:44Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+            }
+
+            for (Thread thread : threadList)
+                thread.join();


The problem with join is that it doesn't rethrow the exception. I personally like to use Executors.fixedThreadPool instead and wait on the Futures.
Currently, if a thread fails with an exception the test will fail with TimeoutException - which is fine, but somewhat enigmatic.

I'll rewrite to ThreadPool.

slukyano · 2018-05-24T19:19:01Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+        /**
+         * Delays initialization slightly to increase chance of catching race condition.
+         */
+        DeclaredBodyEnum() {


I guess it's supposed to be a static block instead of a constructor.

I've tried both, constructor gives better results (more deadlocks).

slukyano · 2018-05-24T19:19:53Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+         * Delays initialization slightly to increase chance of catching race condition.
+         */
+        DeclaredBodyEnum() {
+            AtomicInteger cnt = new AtomicInteger();


Only one thread may execute an initializer (both static and instance), so Atomic isn't needed.
By the way. why not Thread.sleep()?

I think we definitely shouldn't Thread.sleep in clinit. This can have a whole range of side-effects, such as unlocking the class loader. I think busy wait is the way to go.

slukyano · 2018-05-24T19:27:23Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+            ignite.compute(ignite.cluster().forRemotes()).call(new UnmarshalCallable(one, two));
+        }
+        finally {
+            stopAllGrids();


Maybe just use afterTest() instead?

I'll try to do that.

slukyano · 2018-05-24T19:28:39Z

modules/core/src/test/java/org/apache/ignite/marshaller/MarshallerEnumDeadlockMultiJvmTest.java

+
+    /** */
+    private static class OptimizedMarshallerFactory implements Factory<Marshaller> {
+        @Override public Marshaller create() {


I assume these methods complain about missing comments.

I'm not sure about that, do we have to annotate single-method overrides? But I'll add inheritDoc

slukyano · 2018-05-24T19:43:04Z

...c/main/java/org/apache/ignite/internal/marshaller/optimized/OptimizedObjectOutputStream.java

@@ -180,7 +181,7 @@ private void writeObject0(Object obj) throws IOException {
        if (obj == null)
            writeByte(NULL);
        else {
-            if (obj instanceof Throwable && !(obj instanceof Externalizable)) {
+            if (obj instanceof Throwable && !(obj instanceof Externalizable) || BinaryUtils.isEnum(obj.getClass())) {


This needs a code comment explaining why do we care about enums here

I'd rather not see OptimizedMarshaller depend on BinaryMarshaller package. Maybe we have (or need) a generic helper class (ReflectionUtils or something) for that?

Will this fix work with existing persistence files?

There's git annotate :)

Yes I don't like it either, I'll think about it, but I'd rather not create a new class for just 1 method. I'll see where I can stick it.

Yes since we mark this record as JDK (see below) so remote always knows how to de-serialize.

slukyano · 2018-05-25T14:16:58Z

...c/main/java/org/apache/ignite/internal/marshaller/optimized/OptimizedObjectOutputStream.java

@@ -180,7 +181,7 @@ private void writeObject0(Object obj) throws IOException {
        if (obj == null)
            writeByte(NULL);
        else {
-            if (obj instanceof Throwable && !(obj instanceof Externalizable)) {
+            if (obj instanceof Throwable && !(obj instanceof Externalizable) || U.isEnum(obj.getClass())) {


Still think that we should have a comment here on why do we special-case enums. Would be nice to also explain that for throwables, but if we don't know that let's just say "Don't know why Throwable is a special case".

slukyano · 2018-05-25T14:17:31Z

Looks good, but I would like to see a code comment on why do we handle enums specially.

…aller, avoid deadlock.

…shaller, avoid deadlock. - Fixes apache#4063 Signed-off-by: Valentin Kulichenko <valentin.kulichenko@gmail.com>

slukyano reviewed May 25, 2018

View reviewed changes

alamar force-pushed the ignite-8547 branch from 4ddb8bd to 91ca832 Compare May 25, 2018 13:58

slukyano reviewed May 25, 2018

View reviewed changes

IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh…

c2b942f

…aller, avoid deadlock.

alamar force-pushed the ignite-8547 branch from 91ca832 to c2b942f Compare May 25, 2018 14:25

asfgit closed this in 5564a14 May 25, 2018

amirakhmedov pushed a commit to amirakhmedov/ignite that referenced this pull request Jun 5, 2018

IGNITE-8547 - Use JVM serialization for enum values with OptimizedMar…

f34c607

…shaller, avoid deadlock. - Fixes apache#4063 Signed-off-by: Valentin Kulichenko <valentin.kulichenko@gmail.com>

alamar deleted the ignite-8547 branch October 11, 2018 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh… #4063

IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh… #4063

alamar commented May 24, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 24, 2018

alamar May 25, 2018

slukyano May 25, 2018

slukyano commented May 25, 2018

IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh… #4063

IGNITE-8547 Use JVM serialization for enum values with OptimizedMarsh… #4063

Conversation

alamar commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slukyano commented May 25, 2018