Details

    • Type: Improvement
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.1dp
    • Fix Version/s: None
    • Component/s: Core
    • Security Level: Public
    • Labels:
      None
    • Environment:
      all java

      Description

      ViewFuture and HttpFuture have duplicate "wait and check" code to set the status. There is no check for null op for the case op.isCancelled()

      # Subject Project Status CR V
      For Gerrit Dashboard: &For+JCBC-44=message:JCBC-44

        Activity

        Hide
        mikew Mike Wiederhold added a comment -

        Steven,

        I have fixed the op.isCancelled() case to check for null, but we don't have duplicate code in the get() function as you mention. The first "wait" where we count down the latch will trigger a timeout due to the operation not being ready. The second timeout check is checking for an operation that timed out before we were able to call get. Let me give you an example of when this check will be used.

        Imagine your client is doing so many operations that the queue for the client to actually send these operations to the server has grown so long that the operation takes longer than the timeout value to be sent. When this happens we mark the operation as timed out internally and don't send the operation. If you call get() on the operation after this happens then the operation has technically completed so it will miss the "first timeout check". As a result we need to check the operation for timeout once again before we try to get data from it.

        Another simpler example is if you called get() on a ViewFuture/HttpFuture multiple times. The first time the operation would timeout in the "first check" and subsequent calls would cause it to timeout in the second check.

        Show
        mikew Mike Wiederhold added a comment - Steven, I have fixed the op.isCancelled() case to check for null, but we don't have duplicate code in the get() function as you mention. The first "wait" where we count down the latch will trigger a timeout due to the operation not being ready. The second timeout check is checking for an operation that timed out before we were able to call get. Let me give you an example of when this check will be used. Imagine your client is doing so many operations that the queue for the client to actually send these operations to the server has grown so long that the operation takes longer than the timeout value to be sent. When this happens we mark the operation as timed out internally and don't send the operation. If you call get() on the operation after this happens then the operation has technically completed so it will miss the "first timeout check". As a result we need to check the operation for timeout once again before we try to get data from it. Another simpler example is if you called get() on a ViewFuture/HttpFuture multiple times. The first time the operation would timeout in the "first check" and subsequent calls would cause it to timeout in the second check.
        Hide
        SteveC Steven Cooke added a comment -

        Sorry, I was not clear enough. A block of code is literally copied in both classes:

        if (!latch.await(duration, units)) {
        if (op != null)

        { op.timeOut(); }

        status = new OperationStatus(false, "Timed out");
        throw new TimeoutException("Timed out waiting for operation");
        }

        if (op != null && op.hasErrored())

        { status = new OperationStatus(false, op.getException().getMessage()); throw new ExecutionException(op.getException()); }

        if (op.isCancelled())

        { status = new OperationStatus(false, "Operation Cancelled"); throw new ExecutionException(new RuntimeException("Cancelled")); }

        if (op != null && op.isTimedOut())

        { status = new OperationStatus(false, "Timed out"); throw new ExecutionException(new OperationTimeoutException( "Operation timed out.")); }

        This single error needs to be fixed in two places and the fields op, latch, and status are exposed unnecessarily to subclasses.

        Show
        SteveC Steven Cooke added a comment - Sorry, I was not clear enough. A block of code is literally copied in both classes: if (!latch.await(duration, units)) { if (op != null) { op.timeOut(); } status = new OperationStatus(false, "Timed out"); throw new TimeoutException("Timed out waiting for operation"); } if (op != null && op.hasErrored()) { status = new OperationStatus(false, op.getException().getMessage()); throw new ExecutionException(op.getException()); } if (op.isCancelled()) { status = new OperationStatus(false, "Operation Cancelled"); throw new ExecutionException(new RuntimeException("Cancelled")); } if (op != null && op.isTimedOut()) { status = new OperationStatus(false, "Timed out"); throw new ExecutionException(new OperationTimeoutException( "Operation timed out.")); } This single error needs to be fixed in two places and the fields op, latch, and status are exposed unnecessarily to subclasses.
        Hide
        mikew Mike Wiederhold added a comment -

        Good point. I have opened another bug (JCBC-58) to fix the duplicate code issue since this current bug contains two problems and should be split.

        Show
        mikew Mike Wiederhold added a comment - Good point. I have opened another bug ( JCBC-58 ) to fix the duplicate code issue since this current bug contains two problems and should be split.

          People

          • Assignee:
            mikew Mike Wiederhold
            Reporter:
            SteveC Steven Cooke
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Gerrit Reviews

              There are no open Gerrit changes