java3d
  1. java3d
  2. JAVA3D-608

Patch for issue 575: false SceneGraphCycleException raised when Node.checkForCycle() is called from 2 threads

    Details

    • Type: Task Task
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: current
    • Fix Version/s: not determined
    • Component/s: j3d-core
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      608

      Description

      This is a patch for issue 575: false SceneGraphCycleException raised when
      Node.checkForCycle() is called from 2 threads.

      We have a collaborative application where the server sends non live graphs to
      clients. It also writes the graph to disk at an interval. Serialising a large
      graph using the SceneGraphIO will quite often cause the getBounds method to be
      called from two threads at the same time. Which in turn provokes this bug. The
      only disadvantage with this patch is that it will create an ArrayList every time
      checkForCycle() is called. Mostly when the graph is compiled or from
      getBounds(). But I consider correctness to be more important and that you accept
      this patch.

      1. This patch file was generated by NetBeans IDE
      2. This patch can be applied using context Tools: Apply Diff Patch action on
        respective folder.
      3. It uses platform neutral UTF-8 encoding.
      4. Above lines and this line are ignored by the patching process.
        Index: j3d-core/src/classes/share/javax/media/j3d/Node.java
          • j3d-core/src/classes/share/javax/media/j3d/Node.java Base (1.7)
            +++ j3d-core/src/classes/share/javax/media/j3d/Node.java Locally Modified (Based
            On 1.7)
            @@ -31,9 +31,9 @@

      package javax.media.j3d;

      +import java.util.ArrayList;
      import java.util.Hashtable;
      import java.util.Enumeration;
      -import java.lang.reflect.Constructor;

      /**

      • The Node class provides an abstract class for all Group and Leaf Nodes.
        @@ -815,15 +815,15 @@
      • checks for cycles in the scene graph
        */
        void checkForCycle() {
      • if (visited) {
        + ArrayList visitedNodes = new ArrayList();
        + Node parent = this;
        + while (parent != null) {
        + if (visitedNodes.contains(parent)) { throw new SceneGraphCycleException(J3dI18N.getString("Node15")); }
      • visited = true;
      • Node parent = getParent();
      • if (parent != null) { - parent.checkForCycle(); + + visitedNodes.add(parent); + parent = parent.getParent(); }
      • visited = false;
        }
      • }

        Issue Links

          Activity

          Hide
          lamer77 added a comment -

          This patch will fix issue 575

          Show
          lamer77 added a comment - This patch will fix issue 575
          Hide
          aces added a comment -

          Isn't the method synchronization enough to fix it ?
          The ArrayList creation & gc, as well the contains() method calls are expensive.
          For large scene graphs it can be a performance hit.

          Show
          aces added a comment - Isn't the method synchronization enough to fix it ? The ArrayList creation & gc, as well the contains() method calls are expensive. For large scene graphs it can be a performance hit.
          Hide
          lamer77 added a comment -

          Yes, synchronizing the method will also fix it. Although, I'm worried there
          could be a deadlock if the there is a cycle.

          Lets say we have two nodes A and B where A is child of B and B is child of A.
          Thread1 calls A.checkForCycle()
          Thread2 calls B.checkForCycle()
          A.checkForCycle tries to call B.checkForCycle() but must wait on Thread2
          B.checkForCycle tries to call A.checkForCycle() but must wait on Thread1

          Unless we synchronize on a static mutex. Or am I wrong?

          Show
          lamer77 added a comment - Yes, synchronizing the method will also fix it. Although, I'm worried there could be a deadlock if the there is a cycle. Lets say we have two nodes A and B where A is child of B and B is child of A. Thread1 calls A.checkForCycle() Thread2 calls B.checkForCycle() A.checkForCycle tries to call B.checkForCycle() but must wait on Thread2 B.checkForCycle tries to call A.checkForCycle() but must wait on Thread1 Unless we synchronize on a static mutex. Or am I wrong?
          Hide
          lamer77 added a comment -

          Here is different patch that synchronizes the checkForCycle method on a static
          mutex. The advantage is:
          1) No garbage will be created
          2) There is no chance of deadlock

          The only disadvantage this method has is that if two threads try to invoke
          checkForCycle at the same time, one of the threads has to wait. Even if they are
          working on two completely different graphs. However in real life I don't think
          it will be an issue.

          Also note that checkForCycle is rarely invoked, so I don't think performance is
          a big problem. It is only called when:
          1) cloning a not live node
          2) calling getBounds on a not live node or shape
          3) when compiling a BranchGroup or SharedGroup

          1. This patch file was generated by NetBeans IDE
          2. This patch can be applied using context Tools: Apply Diff Patch action on
            respective folder.
          3. It uses platform neutral UTF-8 encoding.
          4. Above lines and this line are ignored by the patching process.
            Index: j3d-core/src/classes/share/javax/media/j3d/Node.java
              • j3d-core/src/classes/share/javax/media/j3d/Node.java Base (1.7)
                +++ j3d-core/src/classes/share/javax/media/j3d/Node.java Locally Modified (Based
                On 1.7)
                @@ -31,9 +31,9 @@

          package javax.media.j3d;

          +import java.util.ArrayList;
          import java.util.Hashtable;
          import java.util.Enumeration;
          -import java.lang.reflect.Constructor;

          /**

          • The Node class provides an abstract class for all Group and Leaf Nodes.
            @@ -164,7 +164,10 @@
            // for checking for cycles
            private boolean visited = false;

          + // is used to synchronize checkForCycle
          + private static final Object checkForCycleMutex = new Object();

          +
          /**

          • Constructs a Node object with default parameters. The default
          • values are as follows:
            @@ -815,6 +818,7 @@
          • checks for cycles in the scene graph
            */
            void checkForCycle() {
            + synchronized (checkForCycleMutex)
            Unknown macro: { if (visited) { throw new SceneGraphCycleException(J3dI18N.getString("Node15")); }@@ -825,5 +829,5 @@ }

            visited = false;
            }

          • }
            +}

          Show
          lamer77 added a comment - Here is different patch that synchronizes the checkForCycle method on a static mutex. The advantage is: 1) No garbage will be created 2) There is no chance of deadlock The only disadvantage this method has is that if two threads try to invoke checkForCycle at the same time, one of the threads has to wait. Even if they are working on two completely different graphs. However in real life I don't think it will be an issue. Also note that checkForCycle is rarely invoked, so I don't think performance is a big problem. It is only called when: 1) cloning a not live node 2) calling getBounds on a not live node or shape 3) when compiling a BranchGroup or SharedGroup This patch file was generated by NetBeans IDE This patch can be applied using context Tools: Apply Diff Patch action on respective folder. It uses platform neutral UTF-8 encoding. Above lines and this line are ignored by the patching process. Index: j3d-core/src/classes/share/javax/media/j3d/Node.java j3d-core/src/classes/share/javax/media/j3d/Node.java Base (1.7) +++ j3d-core/src/classes/share/javax/media/j3d/Node.java Locally Modified (Based On 1.7) @@ -31,9 +31,9 @@ package javax.media.j3d; +import java.util.ArrayList; import java.util.Hashtable; import java.util.Enumeration; -import java.lang.reflect.Constructor; /** The Node class provides an abstract class for all Group and Leaf Nodes. @@ -164,7 +164,10 @@ // for checking for cycles private boolean visited = false; + // is used to synchronize checkForCycle + private static final Object checkForCycleMutex = new Object(); + /** Constructs a Node object with default parameters. The default values are as follows: @@ -815,6 +818,7 @@ checks for cycles in the scene graph */ void checkForCycle() { + synchronized (checkForCycleMutex) Unknown macro: { if (visited) { throw new SceneGraphCycleException(J3dI18N.getString("Node15")); }@@ -825,5 +829,5 @@ } visited = false; } } +}

            People

            • Assignee:
              java3d-issues
              Reporter:
              lamer77
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated: