ViewFuture and HttpFuture may throw NPE

Mike Wiederhold added a comment - 12/May/12 11:40 PM 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.

Steven Cooke added a comment - 13/May/12 9:17 PM 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.

Mike Wiederhold added a comment - 03/Jun/12 8:54 PM 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.