From 98f1ff05580f85debaa245addd02777fe9e68273 Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Wed, 27 Apr 2016 18:16:17 -0700 Subject: [PATCH] Don't wedge full data backups by blocking the data consumer thread In particular, don't ask the producer about error overrides when it is still relying on the consumer to do its job first. This needs to be policy for *any* transport-side error condition, not just the one that was previously handled safely. Any transport- initiated error "on the fly" means that the app-facing side of the engine doesn't know to stop feeding data, and mustn't be consulted with any blocking request. We also now detect unexpected PACKAGE_REJECTED by the transport after data streaming has begun, and translate that to the general TRANSPORT_ERROR for correct handling down the line. Bug 28399225 Bug 28375634 Change-Id: I613dc21bc9f2d23e6520eed6c3ac2e9dbc1d88dc --- .../server/backup/BackupManagerService.java | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/services/backup/java/com/android/server/backup/BackupManagerService.java b/services/backup/java/com/android/server/backup/BackupManagerService.java index 95f9e2dc0935b..1b63cd4958dc4 100644 --- a/services/backup/java/com/android/server/backup/BackupManagerService.java +++ b/services/backup/java/com/android/server/backup/BackupManagerService.java @@ -3706,7 +3706,7 @@ public class BackupManagerService { result = BackupTransport.TRANSPORT_OK; } } catch (IOException e) { - Slog.e(TAG, "Error backing up " + mPkg.packageName, e); + Slog.e(TAG, "Error backing up " + mPkg.packageName + ": " + e.getMessage()); result = BackupTransport.AGENT_ERROR; } finally { try { @@ -4466,7 +4466,6 @@ public class BackupManagerService { } } - // If we've lost our running criteria, tell the transport to cancel // and roll back this (partial) backup payload; otherwise tell it // that we've reached the clean finish state. @@ -4484,14 +4483,16 @@ public class BackupManagerService { } } - // TRANSPORT_ERROR here means that we've hit an error that the runner - // doesn't know about, so it's still moving data but we're pulling the + // A transport-originated error here means that we've hit an error that the + // runner doesn't know about, so it's still moving data but we're pulling the // rug out from under it. Don't ask for its result: we already know better // and we'll hang if we block waiting for it, since it relies on us to // read back the data it's writing into the engine. Just proceed with // a graceful failure. The runner/engine mechanism will tear itself - // down cleanly when we close the pipes from this end. - if (backupPackageStatus != BackupTransport.TRANSPORT_ERROR) { + // down cleanly when we close the pipes from this end. Transport-level + // errors take precedence over agent/app-specific errors for purposes of + // determining our course of action. + if (backupPackageStatus == BackupTransport.TRANSPORT_OK) { // We still could fail in backup runner thread, getting result from there. int backupRunnerResult = backupRunner.getBackupResultBlocking(); if (backupRunnerResult != BackupTransport.TRANSPORT_OK) { @@ -4499,10 +4500,14 @@ public class BackupManagerService { // not TRANSPORT_ERROR here, overwrite it. backupPackageStatus = backupRunnerResult; } + } else { + if (MORE_DEBUG) { + Slog.i(TAG, "Transport-level failure; cancelling agent work"); + } } if (MORE_DEBUG) { - Slog.i(TAG, "Done trying to send backup data: result=" + Slog.i(TAG, "Done delivering backup data: result=" + backupPackageStatus); }