Proper cause for Exceptions Deferred is resumed with

[cbruegg]

I really like this library, but it has caused me some headaches related to network errors. Continuations are resumed with the Exception thrown by OkHttp, but unfortunately that Exception contains no indication about where the network request was started. This is because OkHttp of course uses a thread pool and thus the stack trace only includes internal OkHttp code.

With this PR, the stack during the call to await is recorded. In case of a failure, the Exception cause chain is traversed and another Exception that contains the recorded stack trace is appended. This way users of this library will be able to easily figure out which await call in particular has caused an Exception without manually wrapping every call to await in a try-catch block that wraps it in another locally-created Exception.

[JakeWharton]

This library is about to be obviated by square/retrofit#2886 and as a result isn't accepting any changes.

[cbruegg]

Thanks for the heads-up. It should be simple to port it to that version, fortunately. Do you see the value in this change? If yes, then I'd work on a new PR.

[cbruegg]

To be precise, the await functions here are probably having the same issue: The stack trace won't contain the actual call-site.

[JakeWharton]

I don't want asymmetric functionality specific to Kotlin. The same problem exists for Java callers in a variety of adapters.

[cbruegg]

The same problem exists for Java callers in a variety of adapters.

While that's true, I'd argue it's not surprising there since it's callback-based.

call.enqueue(new Callback() {
    @Override
    public void onFailure(Call call, IOException e) {
        // Naturally e does not contain any reference to the first line of this snippet,
        // but this is expected, since e was not thrown and caught at that call-site, but is received
        // as a parameter here.
        e.printStackTrace();
    }

    @Override
    public void onResponse(Call call, final Response response) throws IOException {
      ...
    }
}

On the other hand, consider the following Kotlin code:

try {
  val x = call.await()
} catch (e: Exception) {
  // We catch an Exception here that should stem from the call-site above,
  // but the stack trace contains no references to it. Which means we caught an Exception
  // here that doesn't look like it could be caught here.
  e.printStackTrace()
}

I strongly believe that this violates the principle of least surprise. Am I missing any concrete issues with the approach of appending another cause to the Exception chain that removes this surprising behavior, aside from the fact that Java-users of course couldn't profit from this, since there are no suspending functions in Java?

[JakeWharton]

This is a problem of all truly async coroutine use. Why should we special case it instead of something done in a more general fashion? I'd also be curious of the performance impact since filling in the stacktrace is far from cheap.

[cbruegg]

IIRC Deferred.await preserves the stack trace properly, including the actual callsite, but I'm not entirely sure. But even if so, is "other approaches have the same problem" truly a good argument against fixing it here?

Maybe this could also be solved inside Call.enqueue in OkHttp, that way even Java users could profit from the change.

I assume that the performance overhead is minor compared to the cost of an HTTP call. However, if you're generally open to fixing this problem, I could do some benchmarking. What do you think? Thanks already!

[JakeWharton]

The cost of the HTTP call is on a background thread–not on a the callers thread.

[cbruegg]

How about making it opt-in? Jake Wharton <[email protected]> schrieb am Mo., 8. Okt. 2018, 17:53:

…

The cost of the HTTP call is on a background thread–not on a the callers thread. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKtPcsZHmmRAXEuDKwJF0SLIVXG7h2eks5ui3TggaJpZM4XL7Tj> .

[JakeWharton]

You can file a feature request on Retrofit and it can be discussed there. I'm not going to take an action on it prior to the bare minimum coroutine support landing in Retrofit though.

…

On Mon, Oct 8, 2018 at 12:52 PM Christian Brüggemann < ***@***.***> wrote: How about making it opt-in? Jake Wharton ***@***.***> schrieb am Mo., 8. Okt. 2018, 17:53: > The cost of the HTTP call is on a background thread–not on a the callers > thread. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #30 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAKtPcsZHmmRAXEuDKwJF0SLIVXG7h2eks5ui3TggaJpZM4XL7Tj > > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEEEbWQdhcnmSoFD_WhvUs5YBA0Jozmks5ui4LQgaJpZM4XL7Tj> .