Skip to content

Don't crash when ExPlat API request failed#253

Merged
wzieba merged 4 commits intotrunkfrom
dont_crash_when_api_request_failed
Nov 14, 2024
Merged

Don't crash when ExPlat API request failed#253
wzieba merged 4 commits intotrunkfrom
dont_crash_when_api_request_failed

Conversation

@wzieba
Copy link
Member

@wzieba wzieba commented Nov 14, 2024

Description

This PR wraps Call#execute in try/catch to make sure that SDK doesn't crash the app in case of API connection issues. Internal context: p1731543068879949-slack-C07J5LNP4SF

Testing

  1. Cherry out fabe1d7 and assert that the newly added test throws unhandled UnknownHostException exception
  2. Check out 138df3b, rerun test and see that exception is handled

@wzieba wzieba added the bug label Nov 14, 2024
fetchedAt = clock.currentTimeSeconds(),
anonymousId = anonymousId,
)
try {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to go with a try/catch as this request is short-lived so I don't think we have to support graceful cancellation of this coroutine.

Copy link
Contributor

@MiSikora MiSikora Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow. try/catch and runCatching behave similarly and don't interact with the suspension mechanism. The main difference is that try/catch allows selective exception handling, whereas runCatching catches all exceptions, including CancellationException, which must be re-thrown. However, these two blocks are functionally equivalent:

suspend fun foo(): String = TODO()

val value = try {
  foo()
} catch (e: Throwable) {
  if (e is CancellationException) throw e
  null
}

val value = runCatching { foo() }
  .getOrElse { e ->
    if (e is CancellationException) throw e
    null
  }

In our case, though, it doesn’t matter because we aren't calling any suspending functions. Call.execute(), JsonAdapter.fromJson(), and ResponseBody.source() are all blocking, and we don’t invoke yield(), so there's no opportunity for coroutine cancellation during execution. I think it is perfectly fine here. I'm only confused about the reason of graceful cancellation support as all calls are blocking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try/catch and runCatching behave similarly

I didn't mean runCatching. I rather meant suspendCancellableCoroutine idea you suggested on Slack.

so there's no opportunity for coroutine cancellation during execution.

In the current shape - yes. But if I introduced suspendCancellableCoroutine, it'd look similar to

 suspendCancellableCoroutine { continuation ->
            val call = okHttpClient.newCall(request)
            continuation.invokeOnCancellation { call.cancel() }

            call.enqueue(object : Callback {

which would be cancellable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I didn't get that from the message. Thanks for clarifying.

@wzieba wzieba enabled auto-merge November 14, 2024 12:21
@wzieba wzieba merged commit c019368 into trunk Nov 14, 2024
@wzieba wzieba deleted the dont_crash_when_api_request_failed branch November 14, 2024 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants