Fix bug reporter failing after not finding some files (#1103)

- Make sure we propagate `CancellationException`.
- Make sure we do a cleanup of temp files.
- Make sure we don't re-compress any lingering temp files.
- Don't stop the upload process if we were able to upload some log files, even if we failed to read some others.
This commit is contained in:
Jorge Martin Espinosa 2023-08-21 15:30:16 +02:00 committed by GitHub
parent b36667844d
commit 097b7f28d5
2 changed files with 245 additions and 220 deletions

1
changelog.d/1082.bugfix Normal file
View file

@ -0,0 +1 @@
Fix bug reporter failing after not finding some log files.

View file

@ -40,6 +40,7 @@ import io.element.android.libraries.di.ApplicationContext
import io.element.android.libraries.network.useragent.UserAgentProvider import io.element.android.libraries.network.useragent.UserAgentProvider
import io.element.android.libraries.sessionstorage.api.SessionStore import io.element.android.libraries.sessionstorage.api.SessionStore
import io.element.android.services.toolbox.api.systemclock.SystemClock import io.element.android.services.toolbox.api.systemclock.SystemClock
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.first
import kotlinx.coroutines.launch import kotlinx.coroutines.launch
@ -152,6 +153,8 @@ class DefaultBugReporter @Inject constructor(
// enumerate files to delete // enumerate files to delete
val mBugReportFiles: MutableList<File> = ArrayList() val mBugReportFiles: MutableList<File> = ArrayList()
try {
var serverError: String? = null var serverError: String? = null
var reportURL: String? = null var reportURL: String? = null
withContext(coroutineDispatchers.io) { withContext(coroutineDispatchers.io) {
@ -168,10 +171,10 @@ class DefaultBugReporter @Inject constructor(
if (withDevicesLogs) { if (withDevicesLogs) {
val files = getLogFiles() val files = getLogFiles()
files.mapNotNullTo(gzippedFiles) { f -> files.mapNotNullTo(gzippedFiles) { f ->
if (!mIsCancelled) { when {
compressFile(f) mIsCancelled -> null
} else { f.extension == "gz" -> f
null else -> compressFile(f)
} }
} }
files.deleteAllExceptMostRecent() files.deleteAllExceptMostRecent()
@ -252,9 +255,21 @@ class DefaultBugReporter @Inject constructor(
} }
} }
// add the gzipped files // add the gzipped files, don't cancel the whole upload if only some file failed to upload
var uploadedSomeLogs = false
for (file in gzippedFiles) { for (file in gzippedFiles) {
try {
builder.addFormDataPart("compressed-log", file.name, file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull())) builder.addFormDataPart("compressed-log", file.name, file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()))
uploadedSomeLogs = true
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
Timber.e(e, "## sendBugReport() : fail to attach file ${file.name}")
}
}
if (!uploadedSomeLogs) {
error("Couldn't upload any logs")
} }
mBugReportFiles.addAll(gzippedFiles) mBugReportFiles.addAll(gzippedFiles)
@ -347,6 +362,8 @@ class DefaultBugReporter @Inject constructor(
mBugReportCall = okHttpClient.get().newCall(request) mBugReportCall = okHttpClient.get().newCall(request)
response = mBugReportCall!!.execute() response = mBugReportCall!!.execute()
responseCode = response.code responseCode = response.code
} catch (e: CancellationException) {
throw e
} catch (e: Exception) { } catch (e: Exception) {
Timber.e(e, "response") Timber.e(e, "response")
errorMessage = e.localizedMessage errorMessage = e.localizedMessage
@ -377,6 +394,8 @@ class DefaultBugReporter @Inject constructor(
try { try {
val responseJSON = JSONObject(it) val responseJSON = JSONObject(it)
serverError = responseJSON.getString("error") serverError = responseJSON.getString("error")
} catch (e: CancellationException) {
throw e
} catch (e: JSONException) { } catch (e: JSONException) {
Timber.e(e, "doInBackground ; Json conversion failed") Timber.e(e, "doInBackground ; Json conversion failed")
} }
@ -386,6 +405,8 @@ class DefaultBugReporter @Inject constructor(
if (null == serverError) { if (null == serverError) {
serverError = "Failed with error $responseCode" serverError = "Failed with error $responseCode"
} }
} catch (e: CancellationException) {
throw e
} catch (e: Exception) { } catch (e: Exception) {
Timber.e(e, "## sendBugReport() : failed to parse error") Timber.e(e, "## sendBugReport() : failed to parse error")
} }
@ -403,11 +424,6 @@ class DefaultBugReporter @Inject constructor(
withContext(coroutineDispatchers.main) { withContext(coroutineDispatchers.main) {
mBugReportCall = null mBugReportCall = null
// delete when the bug report has been successfully sent
for (file in mBugReportFiles) {
file.safeDelete()
}
if (null != listener) { if (null != listener) {
try { try {
if (mIsCancelled) { if (mIsCancelled) {
@ -417,11 +433,19 @@ class DefaultBugReporter @Inject constructor(
} else { } else {
listener.onUploadFailed(serverError) listener.onUploadFailed(serverError)
} }
} catch (e: CancellationException) {
throw e
} catch (e: Exception) { } catch (e: Exception) {
Timber.e(e, "## onPostExecute() : failed") Timber.e(e, "## onPostExecute() : failed")
} }
} }
} }
} finally {
// delete the generated files when the bug report process has finished
for (file in mBugReportFiles) {
file.safeDelete()
}
}
} }
/** /**