Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1467

Improve close() operations on the Java driver

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.1.3
    • Fix Version/s: 3.1.5, 3.2.3
    • Component/s: driver
    • Labels:
      None

      Description

      A few big problems:

      1. Client will hang if submitting a request after close()
      2. Multiple calls to close() hangs periodically
      3. Closing the Cluster doesn't attempt to clean up open Client instances.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/441

          TINKERPOP-1467 Corrected a number of problems in close() operations for the driver [tp31]

          https://issues.apache.org/jira/browse/TINKERPOP-1467

          This was more of a commit than I wanted for `tp31`, but `close()` was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the `Cluster` makes an attempt to clean up any `Client` instances that it spawns.

          Tested with `mvn clean install` and endlessly with `mvn verify -pl gremlin-server -DskipIntegrationTests=false` (basically ran it for a whole day over and over again).

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1467

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/441.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #441


          commit c3acce6e0a8c9270a10ed0eb4f3fd3f46539a655
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-28T15:06:14Z

          Corrected a number of problems in close() operations for the driver.

          This was more of a commit than I wanted for tp31, but close() was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the Cluster makes an attempt to clean up any Client instances that it spawns.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/441 TINKERPOP-1467 Corrected a number of problems in close() operations for the driver [tp31] https://issues.apache.org/jira/browse/TINKERPOP-1467 This was more of a commit than I wanted for `tp31`, but `close()` was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the `Cluster` makes an attempt to clean up any `Client` instances that it spawns. Tested with `mvn clean install` and endlessly with `mvn verify -pl gremlin-server -DskipIntegrationTests=false` (basically ran it for a whole day over and over again). VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1467 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/441.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #441 commit c3acce6e0a8c9270a10ed0eb4f3fd3f46539a655 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-28T15:06:14Z Corrected a number of problems in close() operations for the driver. This was more of a commit than I wanted for tp31, but close() was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the Cluster makes an attempt to clean up any Client instances that it spawns.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user spmallette opened a pull request:

          https://github.com/apache/tinkerpop/pull/442

          TINKERPOP-1467 Corrected a number of problems in close() operations for the driver [master]

          https://issues.apache.org/jira/browse/TINKERPOP-1467

          This work was from `tp31` and was pretty heavily conflicted so I decided on a separate PR. Please see #441 for more information.

          VOTE +1

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/apache/tinkerpop TINKERPOP-1467-master

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/tinkerpop/pull/442.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #442


          commit 934054f83a344ba32db3bcb8617c61340e2087a5
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-28T15:06:14Z

          Corrected a number of problems in close() operations for the driver.

          This was more of a commit than I wanted for tp31, but close() was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the Cluster makes an attempt to clean up any Client instances that it spawns.

          commit 6c84a71f5b50ba9e8a96b8ec766f26f1c8132b72
          Author: Stephen Mallette <spmva@genoprime.com>
          Date: 2016-09-28T18:05:27Z

          Merge remote-tracking branch 'origin/TINKERPOP-1467' into TINKERPOP-1467-master

          Conflicts:
          CHANGELOG.asciidoc
          gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java
          gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java
          gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java
          gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java
          gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user spmallette opened a pull request: https://github.com/apache/tinkerpop/pull/442 TINKERPOP-1467 Corrected a number of problems in close() operations for the driver [master] https://issues.apache.org/jira/browse/TINKERPOP-1467 This work was from `tp31` and was pretty heavily conflicted so I decided on a separate PR. Please see #441 for more information. VOTE +1 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/tinkerpop TINKERPOP-1467 -master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/tinkerpop/pull/442.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #442 commit 934054f83a344ba32db3bcb8617c61340e2087a5 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-28T15:06:14Z Corrected a number of problems in close() operations for the driver. This was more of a commit than I wanted for tp31, but close() was really messed up. Fixed a number of race conditions and other logic that would allow the driver to hang on close. Also made it so that the Cluster makes an attempt to clean up any Client instances that it spawns. commit 6c84a71f5b50ba9e8a96b8ec766f26f1c8132b72 Author: Stephen Mallette <spmva@genoprime.com> Date: 2016-09-28T18:05:27Z Merge remote-tracking branch 'origin/ TINKERPOP-1467 ' into TINKERPOP-1467 -master Conflicts: CHANGELOG.asciidoc gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Cluster.java gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Connection.java gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/441#discussion_r81113830

          — Diff: docs/src/upgrade/release-3.1.x-incubating.asciidoc —
          @@ -27,6 +27,23 @@ TinkerPop 3.1.5

          Release Date: NOT OFFICIALLY RELEASED YET

          +Please see the link:https://github.com/apache/tinkerpop/blob/3.1.4/CHANGELOG.asciidoc#tinkerpop-315-release-date-XXXXXXXXXXXX[changelog] for a complete list of all the modifications that are part of this release.
          +
          +Upgrading for Users
          +~~~~~~~~~~~~~~~~~~~
          +
          +Java Driver and close()
          +^^^^^^^^^^^^^^^^^^^^^^^
          +
          +There were a few problems noted around the `close()` of `Cluster` and `Client` instances, including issues that
          +presented as system hangs. These issues have been resolved, however, it is worth nothing that an unchecked exception
          — End diff –

          worth nothing => worth noting

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/441#discussion_r81113830 — Diff: docs/src/upgrade/release-3.1.x-incubating.asciidoc — @@ -27,6 +27,23 @@ TinkerPop 3.1.5 Release Date: NOT OFFICIALLY RELEASED YET +Please see the link: https://github.com/apache/tinkerpop/blob/3.1.4/CHANGELOG.asciidoc#tinkerpop-315-release-date-XXXXXXXXXXXX[changelog ] for a complete list of all the modifications that are part of this release. + +Upgrading for Users +~~~~~~~~~~~~~~~~~~~ + +Java Driver and close() +^^^^^^^^^^^^^^^^^^^^^^^ + +There were a few problems noted around the `close()` of `Cluster` and `Client` instances, including issues that +presented as system hangs. These issues have been resolved, however, it is worth nothing that an unchecked exception — End diff – worth nothing => worth noting
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on a diff in the pull request:

          https://github.com/apache/tinkerpop/pull/441#discussion_r81113714

          — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java —
          @@ -1248,6 +1250,73 @@ public void shouldProcessSessionRequestsInOrderAfterTimeout() throws Exception {
          }
          }

          + @Test
          + public void shouldCloseAllClientsOnCloseOfCluster() throws Exception {
          + final Cluster cluster = Cluster.open();
          + final Client sessionlessOne = cluster.connect();
          + final Client session = cluster.connect("session");
          + final Client sessionlessTwo = cluster.connect();
          + final Client sessionlessThree = cluster.connect();
          + final Client sessionlessFour = cluster.connect();
          +
          + assertEquals(2, sessionlessOne.submit("1+1").all().get().get(0).getInt());
          + assertEquals(2, session.submit("1+1").all().get().get(0).getInt());
          + assertEquals(2, sessionlessTwo.submit("1+1").all().get().get(0).getInt());
          + assertEquals(2, sessionlessThree.submit("1+1").all().get().get(0).getInt());
          + // dont' send anything on the 4th client
          +
          + // close one of these Clients before the Cluster
          + sessionlessThree.close();
          + cluster.close();
          +
          + try {
          — End diff –

          It is not verified, that exceptions are actually thrown. Should be more like:

          ```
          try

          { sessionXyz.submit("1+1").all().get(); assertTrue(false); }

          catch (Exception ex) {
          ....
          ```

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on a diff in the pull request: https://github.com/apache/tinkerpop/pull/441#discussion_r81113714 — Diff: gremlin-server/src/test/java/org/apache/tinkerpop/gremlin/server/GremlinDriverIntegrateTest.java — @@ -1248,6 +1250,73 @@ public void shouldProcessSessionRequestsInOrderAfterTimeout() throws Exception { } } + @Test + public void shouldCloseAllClientsOnCloseOfCluster() throws Exception { + final Cluster cluster = Cluster.open(); + final Client sessionlessOne = cluster.connect(); + final Client session = cluster.connect("session"); + final Client sessionlessTwo = cluster.connect(); + final Client sessionlessThree = cluster.connect(); + final Client sessionlessFour = cluster.connect(); + + assertEquals(2, sessionlessOne.submit("1+1").all().get().get(0).getInt()); + assertEquals(2, session.submit("1+1").all().get().get(0).getInt()); + assertEquals(2, sessionlessTwo.submit("1+1").all().get().get(0).getInt()); + assertEquals(2, sessionlessThree.submit("1+1").all().get().get(0).getInt()); + // dont' send anything on the 4th client + + // close one of these Clients before the Cluster + sessionlessThree.close(); + cluster.close(); + + try { — End diff – It is not verified, that exceptions are actually thrown. Should be more like: ``` try { sessionXyz.submit("1+1").all().get(); assertTrue(false); } catch (Exception ex) { .... ```
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/441

          As said in a private chat, integration tests were stuck when I ran them first. However, the second attempt succeeded. More investigation is apparently needed, but for this PR:

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/441 As said in a private chat, integration tests were stuck when I ran them first. However, the second attempt succeeded. More investigation is apparently needed, but for this PR: VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/441

          I ran the docker build multiple times today following the report of stuck integration tests. I never ran into the problem, but the adjustments I forced pushed prior to those tests hopefully fixed it.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/441 I ran the docker build multiple times today following the report of stuck integration tests. I never ran into the problem, but the adjustments I forced pushed prior to those tests hopefully fixed it.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/441

          FYI: 3x BUILD SUCCESS in a row.

          My vote still holds true.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/441 FYI: 3x BUILD SUCCESS in a row. My vote still holds true.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user spmallette commented on the issue:

          https://github.com/apache/tinkerpop/pull/442

          I ran the `docker/build.sh -t -n -i` command over and over for the past couple of days and there were no hangs or failures.

          Show
          githubbot ASF GitHub Bot added a comment - Github user spmallette commented on the issue: https://github.com/apache/tinkerpop/pull/442 I ran the `docker/build.sh -t -n -i` command over and over for the past couple of days and there were no hangs or failures.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dkuppitz commented on the issue:

          https://github.com/apache/tinkerpop/pull/442

          Looks good here too.

          VOTE: +1

          Show
          githubbot ASF GitHub Bot added a comment - Github user dkuppitz commented on the issue: https://github.com/apache/tinkerpop/pull/442 Looks good here too. VOTE: +1
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/441

          VOTE +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/441 VOTE +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user okram commented on the issue:

          https://github.com/apache/tinkerpop/pull/442

          VOTE +1.

          Show
          githubbot ASF GitHub Bot added a comment - Github user okram commented on the issue: https://github.com/apache/tinkerpop/pull/442 VOTE +1.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/441

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/441
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/tinkerpop/pull/442

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/tinkerpop/pull/442

            People

            • Assignee:
              spmallette stephen mallette
              Reporter:
              spmallette stephen mallette
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development