Skip to content

Commit

Permalink
[APT-9578] Improve Exception Handling
Browse files Browse the repository at this point in the history 
ArmadilloException needs to support `cause` and `message` in the way `Exception` and most programmers expect them to be used.

Avoids initializing new `Exception()` wherever possible to prevent obfuscating the origin of an error. Never uses `object` for Exceptions, as each carry specific information on their stacktrace.

Exception messages need to be written in plain English, with indication of what component is failing when it fails. And ideally, with a hint on how to fix these errors.
  • Loading branch information
@kabliz
kabliz committed Aug 9, 2024
1 parent f304e58 commit e041fc1
Show file tree
Hide file tree
Showing 15 changed files with 105 additions and 62 deletions.
32 changes: 18 additions & 14 deletions Armadillo/src/main/java/com/scribd/armadillo/ArmadilloPlayerChoreographer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {

override fun beginDownload(audioPlayable: AudioPlayable) {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine not init")))
stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine cannot start download.")))
return
}
downloadEngine.download(audioPlayable)
Expand All @@ -253,7 +253,7 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {

override fun removeAllDownloads() {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("download engine not init")))
stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove all the downloads.")))
return
}
downloadEngine.removeAllDownloads()
Expand Down Expand Up @@ -308,7 +308,10 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
PlaybackState.PLAYING -> controls.pause()
PlaybackState.PAUSED -> controls.play()
else -> {
stateModifier.dispatch(ErrorAction(UnexpectedException("Unknown state: $playbackState")))
stateModifier.dispatch(ErrorAction(
UnexpectedException(cause = IllegalStateException("Neither playing nor paused"),
actionThatFailedMessage = "Trying to play or pause media."))
)
}
}
}
Expand All @@ -333,15 +336,18 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
override fun seekWithinChapter(percent: Int) {
val position = stateProvider.currentState.positionFromChapterPercent(percent)
?: run {
stateModifier.dispatch(ErrorAction(UnexpectedException("No audiobook playing")))
stateModifier.dispatch(ErrorAction(
UnexpectedException(cause = KotlinNullPointerException("Current state's position is null"),
actionThatFailedMessage = "seeking within chapter"))
)
return
}
seekTo(position)
}

override fun removeDownload(audioPlayable: AudioPlayable) {
if (!isDownloadEngineInit) {
stateModifier.dispatch(ErrorAction(EngineNotInitialized("DownloadEngine")))
stateModifier.dispatch(ErrorAction(EngineNotInitialized("Cannot remove a download.")))
return
}
downloadEngine.removeDownload(audioPlayable)
Expand Down Expand Up @@ -412,18 +418,16 @@ internal class ArmadilloPlayerChoreographer : ArmadilloPlayer {
lambda.invoke(controls, playbackState)
} else {
val error = if (controls == null) {
Log.e(TAG, "transportControls null")
TransportControlsNull
Log.e(TAG, "The transport controls are null. Are the controls initialized?")
TransportControlsNull()
} else if (playbackState == null) {
Log.e(TAG, "playbackState is null")
NoPlaybackInfo
Log.e(TAG, "The playbackState is null.")
NoPlaybackInfo()
} else if (PlaybackState.NONE == playbackState) {
Log.e(TAG, "playbackState == PlaybackState.NONE")
InvalidPlaybackState
} else if (!playbackReady) {
EngineNotInitialized("isPlaybackEngineReady == false")
Log.e(TAG, "There is no playback State.")
InvalidPlaybackState()
} else {
UnexpectedException("unknown error in $TAG.doIfPlaybackReady")
EngineNotInitialized("Playback is not ready.")
}

stateModifier.dispatch(ErrorAction(error))
Expand Down
2 changes: 1 addition & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ internal object Reducer {
private fun checkBounds(timeToCheck: () -> Interval<Millisecond>): ArmadilloException? {
return if (maxDurationDiscrepancy >= 0 && timeToCheck.invoke().inSeconds.longValue > maxDurationDiscrepancy) {
Log.e(TAG, "Content metadata is incorrect")
IncorrectChapterMetadataException
IncorrectChapterMetadataException()
} else {
null
}
Expand Down
2 changes: 1 addition & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/StateStore.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,5 +49,5 @@ internal class ArmadilloStateStore(private val reducer: Reducer) :
get() = armadilloStateObservable

override val currentState: ArmadilloState
get() = armadilloStateObservable.value ?: throw MissingDataException("armadilloState should never be null")
get() = armadilloStateObservable.value ?: throw MissingDataException("Armadillo's State should never be null")
}
2 changes: 2 additions & 0 deletions Armadillo/src/main/java/com/scribd/armadillo/Util.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package com.scribd.armadillo

import android.content.Context
import android.os.Build
import androidx.annotation.ChecksSdkIntAtLeast
import com.scribd.armadillo.models.Chapter
import com.scribd.armadillo.time.Interval
import com.scribd.armadillo.time.Millisecond
Expand Down Expand Up @@ -46,4 +47,5 @@ fun sanitizeChapters(chapters: List<Chapter>): List<Chapter> {
}
}

@ChecksSdkIntAtLeast(api = Build.VERSION_CODES.S)
fun hasSnowCone() = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S
7 changes: 4 additions & 3 deletions Armadillo/src/main/java/com/scribd/armadillo/download/DownloadEngine.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import com.scribd.armadillo.StateStore
import com.scribd.armadillo.actions.ErrorAction
import com.scribd.armadillo.download.drm.OfflineDrmManager
import com.scribd.armadillo.error.ArmadilloException
import com.scribd.armadillo.error.ArmadilloIOException
import com.scribd.armadillo.error.DownloadServiceLaunchedInBackground
import com.scribd.armadillo.error.UnexpectedDownloadException
import com.scribd.armadillo.extensions.encodeInByteArray
Expand Down Expand Up @@ -80,15 +81,15 @@ internal class ExoplayerDownloadEngine @Inject constructor(
startDownload(context, request)
} catch (e: Exception) {
if (hasSnowCone() && e is ForegroundServiceStartNotAllowedException) {
stateModifier.dispatch(ErrorAction(DownloadServiceLaunchedInBackground(audioPlayable.id)))
stateModifier.dispatch(ErrorAction(DownloadServiceLaunchedInBackground(audioPlayable.id, e)))
} else {
stateModifier.dispatch(ErrorAction(com.scribd.armadillo.error.ArmadilloIOException(e)))
stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't prepare download.")))
}
}
}

override fun onPrepareError(helper: DownloadHelper, e: IOException) =
stateModifier.dispatch(ErrorAction(com.scribd.armadillo.error.ArmadilloIOException(e)))
stateModifier.dispatch(ErrorAction(ArmadilloIOException(cause = e, whatActionFailedMessage = "Can't report download error.")))
})
}
}
Expand Down
4 changes: 2 additions & 2 deletions Armadillo/src/main/java/com/scribd/armadillo/download/ExoplayerDownloadTracker.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(
} catch (e: IOException) {
Log.e(TAG, "Failed to load downloads", e)
withContext(Dispatchers.Main) {
stateModifier.dispatch(ErrorAction(UnableToLoadDownloadInfo))
stateModifier.dispatch(ErrorAction(UnableToLoadDownloadInfo()))
}
}
}
Expand Down Expand Up @@ -166,7 +166,7 @@ internal class ExoplayerDownloadTracker @Inject constructor(
}

if (taskFailed) {
actions.add(ErrorAction(DownloadFailed))
actions.add(ErrorAction(DownloadFailed()))
}

actions.forEach {
Expand Down
91 changes: 60 additions & 31 deletions Armadillo/src/main/java/com/scribd/armadillo/error/ArmadilloException.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,84 +2,103 @@ package com.scribd.armadillo.error

import com.scribd.armadillo.actions.Action

sealed class ArmadilloException(exception: Exception) : Exception(exception) {
sealed class ArmadilloException(cause: Throwable? = null,
message: String = "Unhandled Armadillo Error")
: Exception(message, cause) {
abstract val errorCode: Int
}

/**
* Errors in Armadillo
*/
data class EngineNotInitialized(val reason: String) : ArmadilloException(exception = Exception(reason)) {
class EngineNotInitialized(message: String)
: ArmadilloException(message = "Armadillo Engine is not initialized: $message") {
override val errorCode = 100
}

object TransportControlsNull : ArmadilloException(Exception()) {
class TransportControlsNull
: ArmadilloException(message = "Internal error, the transport controls are null. Are the controls being initialized?") {
override val errorCode = 101
}

data class MissingDataException(val reason: String) : ArmadilloException(exception = Exception(reason)) {
class MissingDataException(message: String)
: ArmadilloException(message = "Internal error, unexpectedly missing internal data: $message") {
override val errorCode = 102
}

data class ActionBeforeSetup(val action: Action) : ArmadilloException(exception = Exception(action.name)) {
data class ActionBeforeSetup(val action: Action)
: ArmadilloException(message = "Internal Error, using an Action before setup is finished: ${action.name}") {
override val errorCode = 103
}

data class UnrecognizedAction(val action: Action) : ArmadilloException(exception = Exception(action.name)) {
data class UnrecognizedAction(val action: Action)
: ArmadilloException(message = "Internal error, unknown action being taken: ${action.name}") {
override val errorCode = 104
}

object NoPlaybackInfo : ArmadilloException(Exception()) {
class NoPlaybackInfo : ArmadilloException(message = "Internal error, playback info is missing. Is the player initialized?") {
override val errorCode = 105
}

object InvalidPlaybackState : ArmadilloException(Exception()) {
class InvalidPlaybackState
: ArmadilloException(message = "Internal Error, playback state is missing. Has the player been initialized or destroyed?") {
override val errorCode = 106
}

data class InvalidRequest(val reason: String) : ArmadilloException(exception = Exception(reason)) {
class InvalidRequest(message: String)
: ArmadilloException(message = "Internal Error, certain media requests cannot be taken: $message") {
override val errorCode: Int = 107
}

/**
* Playback Errors
*/
data class HttpResponseCodeException(val responseCode: Int, val url: String?, val exception: Exception) : ArmadilloException(exception) {
data class HttpResponseCodeException(val responseCode: Int, val url: String?, override val cause: Exception)
: ArmadilloException(cause = cause, message = "HTTP Error $responseCode.") {
override val errorCode: Int = 200
}

data class ArmadilloIOException(val exception: Exception) : ArmadilloException(exception) {
class ArmadilloIOException(cause: Exception, whatActionFailedMessage: String)
: ArmadilloException(cause = cause, message = "A critical playback issue occurred: $whatActionFailedMessage") {
override val errorCode = 201
}

data class UpdateProgressFailureException(val exception: Exception) : ArmadilloException(exception) {
class UpdateProgressFailureException(cause: Exception)
: ArmadilloException(cause = cause, message = "Progress failed to update suddenly during playback. Download progress or playback " +
"progress may not be reporting correctly.") {
override val errorCode = 202
}

data class PlaybackStartFailureException(val exception: Exception) : ArmadilloException(exception) {
class PlaybackStartFailureException(cause: Exception)
: ArmadilloException(cause = cause, message = "The player is initialized, but it failed to begin.") {
override val errorCode = 203
}

data class ActionListenerException(val exception: Exception) : ArmadilloException(exception) {
class ActionListenerException(cause: Exception)
: ArmadilloException(cause = cause, message = "A problem occurred processing state updates.") {
override val errorCode = 204
}

object IncorrectChapterMetadataException : ArmadilloException(Exception()) {
class IncorrectChapterMetadataException
: ArmadilloException(message = "The metadata for chapter durations are wrong and cannot be used for this content.") {
override val errorCode = 205
}

/**
* Download Errors
*/
data class MissingInfoDownloadException(val reason: String) : ArmadilloException(exception = Exception(reason)) {
class MissingInfoDownloadException(message: String)
: ArmadilloException(message = "Download info is missing: $message") {
override val errorCode = 301
}

object DownloadFailed : ArmadilloException(Exception()) {
class DownloadFailed
: ArmadilloException(message = "The download has failed to finish.") {
override val errorCode = 302
}

object UnableToLoadDownloadInfo : ArmadilloException(Exception()) {
class UnableToLoadDownloadInfo
: ArmadilloException(message = "Failed to load download metadata, queued downloads may fail to resume.") {
override val errorCode = 303
}

Expand All @@ -89,61 +108,71 @@ object UnableToLoadDownloadInfo : ArmadilloException(Exception()) {
* the background before DownloadService.sendAddDownload
*/

data class DownloadServiceLaunchedInBackground(val id: Int) : ArmadilloException(Exception()) {
data class DownloadServiceLaunchedInBackground(val id: Int, override val cause: Exception)
: ArmadilloException(cause = cause,
message = "Android 12 compliance problem: ${cause.message}") {
override val errorCode = 304
}

data class UnexpectedDownloadException(val throwable: Throwable): ArmadilloException(exception = Exception(throwable)){
class UnexpectedDownloadException(throwable: Throwable)
: ArmadilloException(cause = throwable, message = "Unknown problem while downloading."){
override val errorCode = 305
}

/**
* Misc Errors
*/
data class UnexpectedException(val exception: Exception) : ArmadilloException(exception) {
constructor(reason: String) : this(Exception(reason))

class UnexpectedException(cause: Exception, actionThatFailedMessage: String)
: ArmadilloException(cause = cause, message = "Unanticipated error: $actionThatFailedMessage.") {
override val errorCode = 400
}

/** Media Browse Errors */
data class BadMediaHierarchyException(val reason: String) : ArmadilloException(exception = Exception(reason)) {
class BadMediaHierarchyException(cause: Exception?, message: String)
: ArmadilloException(cause = cause, message = message) {
override val errorCode: Int = 501
}

/**
* Audio Renderer Errors
*/

data class RendererConfigurationException(val exception: Exception) : ArmadilloException(exception) {
class RendererConfigurationException(cause: Exception)
: ArmadilloException(cause = cause, message = "The audio data buffer (AudioSink) has been misconfigured.") {
override val errorCode: Int = 601
}

data class RendererInitializationException(val exception: Exception) : ArmadilloException(exception) {
class RendererInitializationException(cause: Exception)
: ArmadilloException(cause = cause, message = "The audio data buffer (AudioSink) isn't initialized.") {
override val errorCode: Int = 602
}

data class RendererWriteException(val exception: Exception) : ArmadilloException(exception) {
class RendererWriteException(cause: Exception)
: ArmadilloException(cause = cause, message = "Cannot write data into the audio data buffer (AudioSink).") {
override val errorCode: Int = 603
}

data class UnknownRendererException(val exception: Exception) : ArmadilloException(exception) {
class UnknownRendererException(cause: Exception)
: ArmadilloException(cause = cause, message = "Unknown problem with the audio data buffer (AudioSink).") {
override val errorCode: Int = 604
}

/**
* DRM errors
*/

data class DrmContentTypeUnsupportedException(val contentType: Int) : ArmadilloException(exception = Exception()) {
data class DrmContentTypeUnsupportedException(val contentType: Int)
: ArmadilloException(message = "This DRM content type is not yet supported. Please consider opening a pull request.") {
override val errorCode = 700
}

data class DrmDownloadException(val exception: Exception) : ArmadilloException(exception) {
class DrmDownloadException(cause: Exception)
: ArmadilloException(cause = cause, "Failed to process DRM license for downloading.") {
override val errorCode = 701
}

data class DrmPlaybackException(val exception: Exception) : ArmadilloException(exception) {
class DrmPlaybackException(cause: Exception)
: ArmadilloException(cause = cause, message = "Cannot play DRM content.") {
override val errorCode = 702
}

3 changes: 2 additions & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/models/ArmadilloState.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ data class ArmadilloState(
return null
}
if (percent > 100 || percent < 0) {
throw UnexpectedException(Exception("Invalid argument: $percent"))
throw UnexpectedException(cause = IllegalArgumentException("Invalid argument: $percent"),
actionThatFailedMessage = "Calculating seekbar progress from percentage.")
}
val currentChapter = playbackInfo.audioPlayable.chapters[playbackInfo.progress.currentChapterIndex]
val decimal = percent / 100.toDouble()
Expand Down
4 changes: 2 additions & 2 deletions Armadillo/src/main/java/com/scribd/armadillo/playback/ExoPlaybackExceptionExt.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
is SocketTimeoutException -> HttpResponseCodeException(0, null, source)
is UnknownHostException ->
HttpResponseCodeException(0, source.message, source) // Message is supposed to be the host for UnknownHostException
else -> ArmadilloIOException(this)
else -> ArmadilloIOException(cause = this, whatActionFailedMessage = "Exoplayer error.")
}
}
} else if (TYPE_RENDERER == type) {
Expand All @@ -40,6 +40,6 @@ internal fun ExoPlaybackException.toArmadilloException(): ArmadilloException {
}
}
} else {
UnexpectedException(this)
UnexpectedException(cause = this, actionThatFailedMessage = "Exoplayer error")
}
}
2 changes: 1 addition & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/playback/MediaSessionConnection.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ internal class MediaSessionConnection(
* completed.
*/
override fun onConnected() {
val mediaBrowserCompat = mediaBrowser ?: throw MissingDataException("media browser should be init")
val mediaBrowserCompat = mediaBrowser ?: throw MissingDataException("Media Browser needs to be initialized.")
val mediaControllerCompat = MediaControllerCompat(context, mediaBrowserCompat.sessionToken)
mediaController = mediaControllerCompat
connectionListener.onConnectionCallback(mediaControllerCompat.transportControls)
Expand Down
Loading

0 comments on commit e041fc1

Please sign in to comment.