Client: Wrap synchronous exceptions #28919

nik9000 · 2018-03-07T02:54:35Z

In the past the Low Level REST Client was super careful not to wrap
any exceptions that it throws from synchronous calls so that callers can
catch the exceptions and work with them. The trouble with that is that
the exceptions are originally thrown on the async thread pool and then
transfered back into calling thread. That means that the stack trace of
the exception doesn't have the calling method which is super ultra
confusing.

This change always wraps exceptions transfered from the async thread
pool so that the stack trace of the thrown exception contains the
caller's stack. It tries to preserve the type of the throw exception but
this is quite a fiddly thing to get right. We have to catch every type
of exception that we want to preserve, wrap with the same type and
rethrow. I've preserved the types of all exceptions that we had tests
mentioning but no other exceptions. The other exceptions are either
wrapped in IOException or RuntimeException.

Closes #28399

In the past the Low Level REST Client was super careful not to wrap any exceptions that it throws from synchronous calls so that callers can catch the exceptions and work with them. The trouble with that is that the exceptions are originally thrown on the async thread pool and then transfered back into calling thread. That means that the stack trace of the exception doesn't have the calling method which is *super* *ultra* confusing. This change always wraps exceptions transfered from the async thread pool so that the stack trace of the thrown exception contains the caller's stack. It tries to preserve the type of the throw exception but this is quite a fiddly thing to get right. We have to catch every type of exception that we want to preserve, wrap with the same type and rethrow. I've preserved the types of all exceptions that we had tests mentioning but no other exceptions. The other exceptions are either wrapped in `IOException` or `RuntimeException`. Closes elastic#28399

nik9000 · 2018-03-07T02:55:53Z

This is a fairly significant departure from the philosophy behind the old code so I'm not at all sure that it is right. I think it is important to "fix" the stack traces in the thrown exception and this seems like the least weird way to do it.

nik9000 · 2018-03-07T02:58:08Z

Ping @elastic/es-core-infra to match the tag, though it'll probably be @javanna doing the review.

Ping other folks that have contributed to the rest client in case they have other ideas: @PnPie @olcbean, @tlrx.

jasontedor

I left a comment about constructing the wrapping exception.

jasontedor · 2018-03-07T15:29:21Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

+                 * like the asynchronous API. We wrap the exception so that the caller's
+                 * signature shows up in any exception we throw.
+                 */
+                if (exception instanceof ResponseException) {


I would have thought to use reflection here to look for a constructor of the appropriate shape (taking an Exception of the same concrete type), falling back to String and calling Throwable#initCause if not, and then falling back to RuntimeException. What do you think of that?

I thought about the reflection solution here and rejected it, mostly because we tend to dislike it in the rest of Elasticsearch. I think the trouble with reflection here is that it is difficult to reason about. Not in the "what is going to happen?" sense, but in the "are the ctors that I call going to do the right thing with the things I give them?" sense.

For what it is worth I'd like to abstract async http client from the Elasticsearch client eventually and I don't think the reflection based approach would play well with that.

I certainly understand that this will change the exceptions that we throw in the cases where I don't have an explicit branch though.

Or is it a way to wrap the exceptions thrown (all or parts) already in the async call into a "CustomException" ? kind of like the ElasticsearchException ? This might make exception a little bit redundant but it keeps the consistency of the 2 sides of sync and async API. (If the exception parse back stuff in sync listener is too tricky to do) ?

I think the exception unwrapping is going to be tricky to get perfect. I'd be ok with some kind of rethrown exception. I mean, ultimately, that is what we end up with if we fall through all the instanceof blocks. We could save ourselves the trouble and wrap the same way every time. I'm honestly not sure what'd be easier for callers though.

@javanna, what do you think? You've been working in this area a lot longer than I have.

As far as I understand what it all boils down to here is the different IOExceptions that can be thrown by the underlying http client, there may be more than the ones that we have a branch for.

I think the current solution is good enough, I even wonder if anybody is ever going to catch socket timeout rather connect timeout etc. maybe all those could just be generic IOExceptions like we already do below and one has to look at the cause to see what it really is? I think that would be reasonable too, but we do want to rethrow ResponseException as a proper ResponseException as it's our own (like we already do).

I do not have anything against the proposed reflection solution either, but maybe what we have now is easier to reason about and we can always add branches if we missed something.

Maybe we should document what we do with exceptions in the sync methods, so people know what to expect.

another idea, not sure though how much it makes sense, could be to add our own specific runtime exception (instead of a generic one) with a good name that indicates that it's used only for wrapping, and always use that one, and document well that users have to look at the cause. Then we may want to also remove all the throws IOException from the performRequest methods. Not sure though, maybe this is the silliest way of addressing this problem, it is hard to evaluate how much this would change things for users, I suspect not much but not sure.

I keep coming back to "I'd like to make async httpclient an implementation detail one day" and I think this kind of if instanceof then rethrow is the kind of logic I'm going to need to do that one day. So, I think I'll stick with this approach for entirely selfish reasons. As a side effect, I think it should mostly be transparent to users. Except for branches I haven't caught....

++ on adding javadoc for this.

@nik9000 do you think it would be helpful to add another branch for SSLException ?

@nik9000 do you think it would be helpful to add another branch for SSLException ?

Fine with me! I can do it.

olcbean

Looks good and would bring more useful info 👍

I just have one general concern : is it possible that an exception type ( explicitly expected by the user ) is wrapped into the general RuntimeException ? And then the use will need to change their code.
IMHO going with reflection should minimize the risk.

olcbean · 2018-03-07T20:38:35Z

client/rest/src/test/java/org/elasticsearch/client/RestClientMultipleHostsTests.java

-                }
+                exec.execute(new Runnable() {
+                    @Override
+                    public void run() {


Runnable is a FI, so maybe you can use exec.execute(() -> { .... } );

I believe that I can't do that here because these tests are compiled with source compatibility set to 1.7.

Oh..
Is there a reason why the test are compiled against 1.7 ? 😕

The non-test code has to be compiled against 1.7 because we want folks still on 1.7 to be able to use it. The tests kind of come along for the ride mostly to make sure that everything works properly in 1.7. For things like that it is a bit of a pain though.

olcbean · 2018-03-07T20:45:58Z

client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java

+            if (se.getClassName().equals(myMethod.getClassName())
+                    && se.getMethodName().equals(myMethod.getMethodName())) {
+                foundMyMethod = true;
+                break;


What about removing foundMyMethod and directly return here? Then the following if check can be removed as well

Yeah! I had originally had this as part of a larger method so return wasn't possible. But now that I've pulled it out return is much better. Will push a change for that.

olcbean · 2018-03-07T20:49:58Z

client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java

+        if (false == foundMyMethod) {
+            StringWriter stack = new StringWriter();
+            e.printStackTrace(new PrintWriter(stack));
+            fail("didn't find my stack trace (looks like " + myMethod + ") in\n" + stack);


nit : maybe change the msg to something that is explaining why you expect to find myMethod ?

I'll reword and add javadoc to the method to make it more clear why we call it. I don't want to write a whole paragraph in the exception message because I think it'd be hard to read. But I think folks will see the method's javadoc.

olcbean · 2018-03-07T21:01:08Z

client/rest/src/test/java/org/elasticsearch/client/SyncResponseListenerTests.java

+        try {
+            syncResponseListener.get();
+            fail("get should have failed");
+        } catch(IOException e) {


nit : missing space
s/catch(IOException e)/catch (IOException e)/
I see this at several places in this file. Maybe reformat the new code ?

I'll reform on the files that I touched I think.

olcbean · 2018-03-07T21:04:44Z

client/rest/src/test/java/org/elasticsearch/client/RestClientSingleHostTests.java

+                                @Override
+                                public void run() {
+                                    futureCallback.completed(httpResponse);
+                                }


nit : maybe simply call

exec.execute(() -> futureCallback.completed(httpResponse));

Same deal with source compatibility as above.

olcbean · 2018-03-07T21:09:16Z

client/rest/src/test/java/org/elasticsearch/client/RestClientTests.java

+                @Override
+                public void onFailure(Exception exception) {
+                    assertThat(exception, instanceOf(UnsupportedOperationException.class));
+                    assertEquals("http method not supported: unsupported", exception.getMessage());


off topic : I see that both assertThat and assertEquals are used throughout the tests. Is there a preference to use one over the other ( for new code ) ?

Depends on who you ask. I prefer to use the purpose built assertions when they are the same as the ones from Hamcrest because they are fairly terse and don't need to make objects [1]. I prefer to use the hamcrest ones when they offer better error reporting. I rarely go out of my way to write new hamcrest matchers. By that logic, I could have written assertThat(e, hasMessage(equalTo("http method not supported: unsupported"))); then I would have done so. But hasMessage isn't a thing.

Other folks always use assertThat because has very obvious parameter ordering that reads well and the compile can check it. This is a totally valid argument and one that I'll probably end up being swayed by eventually.

I tend not to rewrite assertions that someone else wrote unless I have a good reason to.

[1]: I imagine hamcrest matchers are a great candidate for the JVM to optimize to stack based allocation but I figure that only kicks in for hot method and most test methods aren't run enough to be hot.

Trying to only touch the files i touched.

nik9000 · 2018-03-08T20:31:55Z

Thanks for the reviews @jasontedor and @olcbean! I've pushed some changes for @olcbean's comments and left some responses. The reflection comment is the biggest sticking point here I think. I've just had bad experiences with that sort of thing so I didn't think to much about it. I've thought more about it now that you both suggested it but I still don't really like it for reasons I added in a reply to the inline comment about it.

javanna

I left some thoughts and ideas, I think the most important thing is that we address this one way or the other, I don't have a strong opinion on how :) thanks for looking into this @nik9000

javanna · 2018-03-16T09:28:20Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

@@ -203,6 +203,14 @@ public Response performRequest(String method, String endpoint, Map<String, Strin
     * they previously failed (the more failures, the later they will be retried). In case of failures all of the alive nodes (or dead
     * nodes that deserve a retry) are retried until one responds or none of them does, in which case an {@link IOException} will be thrown.
     *
+     * This method works by performing an asynchronous call and the waiting


remove "the" ?

javanna · 2018-03-16T09:28:33Z

client/rest/src/main/java/org/elasticsearch/client/RestClient.java

@@ -203,6 +203,14 @@ public Response performRequest(String method, String endpoint, Map<String, Strin
     * they previously failed (the more failures, the later they will be retried). In case of failures all of the alive nodes (or dead
     * nodes that deserve a retry) are retried until one responds or none of them does, in which case an {@link IOException} will be thrown.
     *
+     * This method works by performing an asynchronous call and the waiting
+     * for the result. If the asynchronous call throws and exception we wrap


nik9000 · 2018-03-16T16:43:36Z

@elasticmearchin, retest this please.

In the past the Low Level REST Client was super careful not to wrap any exceptions that it throws from synchronous calls so that callers can catch the exceptions and work with them. The trouble with that is that the exceptions are originally thrown on the async thread pool and then transfered back into calling thread. That means that the stack trace of the exception doesn't have the calling method which is *super* *ultra* confusing. This change always wraps exceptions transferred from the async thread pool so that the stack trace of the thrown exception contains the caller's stack. It tries to preserve the type of the throw exception but this is quite a fiddly thing to get right. We have to catch every type of exception that we want to preserve, wrap with the same type and rethrow. I've preserved the types of all exceptions that we had tests mentioning but no other exceptions. The other exceptions are either wrapped in `IOException` or `RuntimeException`. Closes #28399

* master: (476 commits) Fix compilation errors in ML integration tests Small code cleanups and refactorings in persistent tasks (elastic#29109) Update allocation awareness docs (elastic#29116) Configure error file for archive packages (elastic#29129) Configure heap dump path for archive packages (elastic#29130) Client: Add missing test getMinGenerationForSeqNo should acquire read lock (elastic#29126) Backport - Do not renew sync-id PR to 5.6 and 6.3 Client: Wrap SSLHandshakeException in sync calls Fix creating keystore when upgrading (elastic#29121) Align thread pool info to thread pool configuration (elastic#29123) TEST: Adjust translog size assumption in new engine Docs: HighLevelRestClient#multiGet (elastic#29095) Client: Wrap synchronous exceptions (elastic#28919) REST: Clear Indices Cache API simplify param parsing (elastic#29111) Fix typo in ExceptionSerializationTests Remove BWC layer for rejected execution exception Fix EsAbortPolicy to conform to API (elastic#29075) [DOCS] Removed prerelease footnote from upgrade table. Docs: Support triple quotes (elastic#28915) ...

nik9000 added review :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch v7.0.0 v6.3.0 labels Mar 7, 2018

nik9000 requested review from tlrx and javanna March 7, 2018 02:54

nik9000 added the >feature label Mar 7, 2018

jasontedor reviewed Mar 7, 2018

View reviewed changes

olcbean reviewed Mar 7, 2018

View reviewed changes

nik9000 added 4 commits March 8, 2018 12:23

Fixes from comments

d823914

another space

fdabee2

More catch (

966bdde

Trying to only touch the files i touched.

Undo changes on files commit doesnt' touch

17c10f0

javanna approved these changes Mar 15, 2018

View reviewed changes

nik9000 added 4 commits March 15, 2018 15:30

Merge branch 'master' into rest_client_stack

888c6bd

Javadoc

b5fa54c

Merge branch 'master' into rest_client_stack

6b3e0b5

WIP

990444a

javanna reviewed Mar 16, 2018

View reviewed changes

nik9000 added 2 commits March 16, 2018 10:05

Better words

7212e84

Test for wrapping exception

fa47ddb

nik9000 merged commit 60cb476 into elastic:master Mar 16, 2018

nik9000 added the backport pending label Mar 16, 2018

nik9000 removed the backport pending label Mar 17, 2018

nik9000 mentioned this pull request Jul 10, 2018

Switch reindex tests to new style requests #31941

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Client: Wrap synchronous exceptions #28919

Client: Wrap synchronous exceptions #28919

Conversation

nik9000 commented Mar 7, 2018

nik9000 commented Mar 7, 2018

nik9000 commented Mar 7, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PnPie Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Mar 8, 2018

javanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Mar 16, 2018

PnPie Mar 8, 2018 •

edited

Loading