commit 02dc6554bd3aa9cae0993deda831c170cb3a3693
author Gilad Arnold <garnold@chromium.org> Thu Nov 14 11:27:54 2013 -0800
committer Mike Frysinger <vapier@chromium.org> Mon Nov 18 07:24:45 2013 +0000
tree dc8ea636c8c265fb783f93e84586b843a1cc79cf
parent 2136ce1397bab98e9fe09d351c683c07d513eb53

devserver: fix exception related discrepancies

* DeltaPayloadsArtifact._Setup() neglected to (re-)raise and exception
  when an error occurred. This contradicts the docstring and a likely
  cause for inconsistencies surrounding missing artifact files.

* BuildArtifact.Process() caught and re-raised any exception, although
  it's only supposed to be raising BuildArtifactError. We now convert
  any unknown exception into the latter (albeit losing the stack trace).
  This necessitates that we adjust our handling of exception at call
  sites for build_artifact, too.

BUG=chromium:277839
TEST=Unit tests.

Change-Id: I30a87e9722460fdbfab54c3ff60e1d7a8013b3cb
Reviewed-on: https://chromium-review.googlesource.com/176932
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Dan Shi <dshi@chromium.org>

build_artifact.py

diff --git a/build_artifact.py b/build_artifact.py
index bb8ec46..38d8d59 100755
--- a/build_artifact.py
+++ b/build_artifact.py
@@ -172,7 +172,7 @@
     with open(os.path.join(self.install_dir, self.marker_name), 'w') as f:
       f.write('\n'.join(self.installed_files))
 
-  def WaitForArtifactToExist(self, timeout, update_name=True):
+  def _WaitForArtifactToExist(self, timeout, update_name=True):
     """Waits for artifact to exist and sets self.name to appropriate name.
 
     Args:
@@ -274,14 +274,19 @@
           # If the artifact should already have been uploaded, don't waste
           # cycles waiting around for it to exist.
           timeout = 1 if no_wait else 10
-          self.WaitForArtifactToExist(timeout)
+          self._WaitForArtifactToExist(timeout)
           self._Download()
           self._Setup()
           self._MarkArtifactStaged()
         except Exception as e:
           # Save the exception to a file for downloader.IsStaged to retrieve.
           self._SaveException(e)
-          raise
+
+          # Convert an unknown exception into an ArtifactDownloadError.
+          if type(e) is ArtifactDownloadError:
+            raise
+          else:
+            raise ArtifactDownloadError('An error occurred: %s' % e)
       else:
         self._Log('%s is already staged.', self)
 
@@ -371,6 +376,7 @@
         self.installed_files.append(stateful_update_symlink)
       except ArtifactDownloadError as e:
         self._Log('Could not process %s: %s', artifact, e)
+        raise
 
 
 class BundledBuildArtifact(BuildArtifact):

build_artifact_unittest.py

diff --git a/build_artifact_unittest.py b/build_artifact_unittest.py
index 46e9249..eb5bf4a 100755
--- a/build_artifact_unittest.py
+++ b/build_artifact_unittest.py
@@ -167,8 +167,8 @@
     artifact.staging_dir = install_dir
     self.mox.StubOutWithMock(subprocess, 'check_call')
     subprocess.check_call(mox.In('autotest/utils/packager.py'), cwd=install_dir)
-    self.mox.StubOutWithMock(artifact, 'WaitForArtifactToExist')
-    artifact.WaitForArtifactToExist(1)
+    self.mox.StubOutWithMock(artifact, '_WaitForArtifactToExist')
+    artifact._WaitForArtifactToExist(1)
     artifact._Download()
     artifact._Extract()
     self.mox.ReplayAll()

downloader.py

diff --git a/downloader.py b/downloader.py
index 1177db9..9b3f5cb 100755
--- a/downloader.py
+++ b/downloader.py
@@ -7,7 +7,6 @@
 
 import build_artifact
 import common_util
-import gsutil_util
 import log_util
 
 
@@ -102,11 +101,11 @@
 
   @staticmethod
   def _TryRemoveStageDir(directory_path):
-    """If download failed with GSUtilError, try to remove the stage dir.
+    """If download failed, try to remove the stage dir.
 
-    If the download attempt failed with GSUtilError and staged.timestamp is the
-    only file in that directory. The build could be non-existing, and the
-    directory should be removed.
+    If the download attempt failed (ArtifactDownloadError) and staged.timestamp
+    is the only file in that directory. The build could be non-existing, and
+    the directory should be removed.
 
     @param directory_path: directory used to stage the image.
 
@@ -131,7 +130,7 @@
      async: If True, return without waiting for download to complete.
 
     Raises:
-      gsutil_util.GSUtilError: If we failed to download the artifact.
+      build_artifact.ArtifactDownloadError: If failed to download the artifact.
 
     """
     common_util.MkDirP(self._build_dir)
@@ -197,7 +196,7 @@
     try:
       for artifact in artifacts:
         artifact.Process(no_wait)
-    except gsutil_util.GSUtilError:
+    except build_artifact.ArtifactDownloadError:
       Downloader._TryRemoveStageDir(self._build_dir)
       raise