rishisankar commented on a change in pull request #1770: URL: https://github.com/apache/lucene-solr/pull/1770#discussion_r474823357
########## File path: solr/solrj/src/java/org/apache/solr/client/solrj/impl/Http2SolrClient.java ########## @@ -394,26 +393,26 @@ public void onHeaders(Response response) { assert ObjectReleaseTracker.track(is); try { NamedList<Object> body = processErrorsAndResponse(solrRequest, parser, response, is); - asyncListener.onSuccess(body); - } catch (RemoteSolrException e) { - if (SolrException.getRootCause(e) != CANCELLED_EXCEPTION) { - asyncListener.onFailure(e); - } - } catch (SolrServerException e) { - asyncListener.onFailure(e); + future.complete(body); + } catch (RemoteSolrException | SolrServerException e) { + future.completeExceptionally(e); } }); } @Override public void onFailure(Response response, Throwable failure) { super.onFailure(response, failure); - if (failure != CANCELLED_EXCEPTION) { - asyncListener.onFailure(new SolrServerException(failure.getMessage(), failure)); - } + future.completeExceptionally(new SolrServerException(failure.getMessage(), failure)); } }); - return () -> req.abort(CANCELLED_EXCEPTION); + future.exceptionally((error) -> { + if (error instanceof CancellationException) { + req.abort(new Exception()); Review comment: > * Why do an instanceof check on "error" at all; why not simply always req.abort(error)? > * Why create a new Exception when we already have a suitable Throwable type in variable "error"? Yes I think you're right, the instanceof check and new Exception seem unneccessary - will change that > I see your point, but I in this case from CompletableFuture perspective, it get called with `cancel()` first then `completeExceptionally` will that mess up with internal state of the future instance? Hmm I don't think so - the `future.exceptionally` is a hook that would run after the future is already marked as completed due to an exception, so subsequent calls to `completeExceptionally` shouldn't do anything. From CF javadoc, "When two or more threads attempt to complete, completeExceptionally, or cancel a CompletableFuture, only one of them succeeds." ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org