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

Enforce AutoCloseable Semantics on Transaction

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Implemented
    • Affects Version/s: 3.0.1-incubating
    • Fix Version/s: 3.1.0-incubating
    • Component/s: structure
    • Labels:

      Description

      TINKERPOP3-764 unified semantics of the Transaction.close() method. The original thought was to deprecate close but discussions on the dev mailing list:

      http://mail-archives.apache.org/mod_mbox/incubator-tinkerpop-dev/201508.mbox/%3CCAA-H4380Hc9H8XU7hx%2BwsXRG_cBqLwYDuEk7HiOgwj-xe8CLkg%40mail.gmail.com%3E

      made it apparent that it was better to implement AutoCloseable on Transaction and further enforce those semantics, specifically - enforce the default close behavior as "rollback" through tests.

        Activity

        Hide
        bryncooke Bryn Cooke added a comment -

        Hi I just want to point out a scenario where having an Autoclosable transaction is useful although I appreciate the current semantics of the method within TP may not support this:

        Ideally I want to be able to do this:

        try (Transaction tx = graph.tx()) {
             //Do stuff
             tx.commit();
        }
        

        Where 'close' on the transaction is equivalent to calling 'rollback'.
        This ensures that I never accidentally leave a transaction open.
        In the case that an exception is thrown in my code I don't have to worry as the transaction is automatically rolled back.

        If we have to use this:

        try {
             //Do stuff
          graph.tx().commit()
        } finally {
          graph.tx().rollback()
        }
        

        then I always have to remember put a rollback statement in a finally block.
        In the end it doesn't save many characters, but I do remember that there were various issues on the gremlin user groups where folks had forgotten to put a rollback in a finally block and had inadvertently left a transaction open resulting in weird behaviour.

        Show
        bryncooke Bryn Cooke added a comment - Hi I just want to point out a scenario where having an Autoclosable transaction is useful although I appreciate the current semantics of the method within TP may not support this: Ideally I want to be able to do this: try (Transaction tx = graph.tx()) { //Do stuff tx.commit(); } Where 'close' on the transaction is equivalent to calling 'rollback'. This ensures that I never accidentally leave a transaction open. In the case that an exception is thrown in my code I don't have to worry as the transaction is automatically rolled back. If we have to use this: try { //Do stuff graph.tx().commit() } finally { graph.tx().rollback() } then I always have to remember put a rollback statement in a finally block. In the end it doesn't save many characters, but I do remember that there were various issues on the gremlin user groups where folks had forgotten to put a rollback in a finally block and had inadvertently left a transaction open resulting in weird behaviour.
        Hide
        spmallette stephen mallette added a comment -

        Updated the description of this issue based on discussions in the dev mailing list.

        Show
        spmallette stephen mallette added a comment - Updated the description of this issue based on discussions in the dev mailing list.
        Hide
        spmallette stephen mallette added a comment -

        Fixed via:

        https://github.com/apache/incubator-tinkerpop/commit/54b114bf6932503026ce777576255d817ef9118e

        Note the test renaming and change from using rollback() for the default of Transaction.close() - those are the potential breaking changes.

        Show
        spmallette stephen mallette added a comment - Fixed via: https://github.com/apache/incubator-tinkerpop/commit/54b114bf6932503026ce777576255d817ef9118e Note the test renaming and change from using rollback() for the default of Transaction.close() - those are the potential breaking changes.

          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