feat: retries for resumable bucket.upload and file.save (#1511)

* feat: retries for resumable bucket.upload and file.save * fix tests, remove unneeded checks
googleapis · Jul 23, 2021 · 9bf163c · 9bf163c
1 parent f313030
commit 9bf163c
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 12 deletions.
diff --git a/src/bucket.ts b/src/bucket.ts
@@ -3725,7 +3725,6 @@ class Bucket extends ServiceObject {
     callback?: UploadCallback
   ): Promise<UploadResponse> | void {
     const upload = () => {
-      const isMultipart = options.resumable === false;
       const returnValue = retry(
         async (bail: (err: Error) => void) => {
           await new Promise<void>((resolve, reject) => {
@@ -3737,7 +3736,6 @@ class Bucket extends ServiceObject {
               .pipe(writable)
               .on('error', err => {
                 if (
-                  isMultipart &&
                   this.storage.retryOptions.autoRetry &&
                   this.storage.retryOptions.retryableErrorFn!(err)
                 ) {

diff --git a/src/file.ts b/src/file.ts
@@ -3618,14 +3618,12 @@ class File extends ServiceObject<File> {
       typeof optionsOrCallback === 'function' ? optionsOrCallback : callback;
     const options =
       typeof optionsOrCallback === 'object' ? optionsOrCallback : {};
-    const isMultipart = options.resumable === false;
     const returnValue = retry(
       async (bail: (err: Error) => void) => {
         await new Promise<void>((resolve, reject) => {
           const writable = this.createWriteStream(options)
             .on('error', err => {
               if (
-                isMultipart &&
                 this.storage.retryOptions.autoRetry &&
                 this.storage.retryOptions.retryableErrorFn!(err)
               ) {

diff --git a/test/bucket.ts b/test/bucket.ts
@@ -2440,6 +2440,24 @@ describe('Bucket', () => {
     });
 
     describe('resumable uploads', () => {
+      class DelayedStream500Error extends Transform {
+        retryCount: number;
+        constructor(retryCount: number) {
+          super();
+          this.retryCount = retryCount;
+        }
+        _transform(chunk: string | Buffer, _encoding: string, done: Function) {
+          this.push(chunk);
+          setTimeout(() => {
+            if (this.retryCount === 1) {
+              done(new HTTPError('first error', 500));
+            } else {
+              done();
+            }
+          }, 5);
+        }
+      }
+
       beforeEach(() => {
         fsStatOverride = (path: string, callback: Function) => {
           callback(null, {size: 1}); // Small size to guarantee simple upload
@@ -2494,6 +2512,62 @@ describe('Bucket', () => {
         };
         bucket.upload(filepath, options, assert.ifError);
       });
+
+      it('should not retry a nonretryable error code', done => {
+        const fakeFile = new FakeFile(bucket, 'file-name');
+        const options = {destination: fakeFile, resumable: true};
+        let retryCount = 0;
+        fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
+          class DelayedStream403Error extends Transform {
+            _transform(
+              chunk: string | Buffer,
+              _encoding: string,
+              done: Function
+            ) {
+              this.push(chunk);
+              setTimeout(() => {
+                retryCount++;
+                if (retryCount === 1) {
+                  done(new HTTPError('first error', 403));
+                } else {
+                  done();
+                }
+              }, 5);
+            }
+          }
+          setImmediate(() => {
+            assert.strictEqual(options_.resumable, true);
+            retryCount++;
+            done();
+          });
+          return new DelayedStream403Error();
+        };
+
+        bucket.upload(filepath, options, (err: Error) => {
+          assert.strictEqual(err.message, 'first error');
+          assert.ok(retryCount === 2);
+          done();
+        });
+      });
+
+      it('resumable upload should retry', done => {
+        const fakeFile = new FakeFile(bucket, 'file-name');
+        const options = {destination: fakeFile, resumable: true};
+        let retryCount = 0;
+        fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
+          setImmediate(() => {
+            assert.strictEqual(options_.resumable, true);
+            retryCount++;
+            done();
+          });
+          return new DelayedStream500Error(retryCount);
+        };
+        bucket.upload(filepath, options, (err: Error) => {
+          assert.strictEqual(err.message, 'first error');
+          assert.ok(retryCount === 1);
+          done();
+        });
+      });
     });
 
     describe('multipart uploads', () => {

diff --git a/test/file.ts b/test/file.ts
@@ -240,7 +240,7 @@ describe('File', () => {
         totalTimeout: 600,
         maxRetryDelay: 60,
         retryableErrorFn: (err: HTTPError) => {
-          return err.code === 500;
+          return err?.code === 500;
         },
       },
     };
@@ -4107,19 +4107,16 @@ describe('File', () => {
         assert.ok(retryCount === 2);
       });
 
-      it('non-multipart upload should not retry', async () => {
+      it('resumable upload should retry', async () => {
         const options = {resumable: true};
         let retryCount = 0;
         file.createWriteStream = () => {
           retryCount++;
           return new DelayedStream500Error(retryCount);
         };
-        try {
-          await file.save(DATA, options);
-          throw Error('unreachable');
-        } catch (e) {
-          assert.strictEqual(e.message, 'first error');
-        }
+
+        await file.save(BUFFER_DATA, options);
+        assert.ok(retryCount === 2);
       });
     });