samples: storage.objects.insert precondition samples (#2047)

* test: add retries (#2039) * refactor: remove unused `restart` private method (#2038) Co-authored-by: Sameena Shaffeeullah <shaffeeullah@google.com> * fix: Retry `EPIPE` Connection Errors + Attempt Retries in Resumable Upload Connection Errors (#2040) * feat: Add `epipe` as retryable error * fix: capture and retry potential connection errors * test: Add tests and remove logs * test: set `retryOptions` by copy rather than reference * fix: grammar * chore(main): release 6.4.1 (#2036) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> * test: add delay to bucket tests to reduce rate limiting errors (#2043) * samples: storage.objects.insert precondition samples * updated test Co-authored-by: Daniel Bankhead <danielbankhead@google.com> Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Denis DelGrosso <85250797+ddelgrosso1@users.noreply.github.com>
googleapis · Aug 18, 2022 · ca1b4b7 · ca1b4b7
1 parent 49fa4d5
commit ca1b4b7
Show file tree

Hide file tree

Showing 14 changed files with 160 additions and 105 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,14 @@
 
 [1]: https://www.npmjs.com/package/@google-cloud/storage?activeTab=versions
 
+## [6.4.1](https://github.com/googleapis/nodejs-storage/compare/v6.4.0...v6.4.1) (2022-08-12)
+
+
+### Bug Fixes
+
+* Remove `pumpify` ([#2029](https://github.com/googleapis/nodejs-storage/issues/2029)) ([edc1d64](https://github.com/googleapis/nodejs-storage/commit/edc1d64069a6038c301c3b775f116fbf69b10b28))
+* Retry `EPIPE` Connection Errors + Attempt Retries in Resumable Upload Connection Errors ([#2040](https://github.com/googleapis/nodejs-storage/issues/2040)) ([ba35321](https://github.com/googleapis/nodejs-storage/commit/ba35321c3fef9a1874ff8fbea43885e2e7746b4f))
+
 ## [6.4.0](https://github.com/googleapis/nodejs-storage/compare/v6.3.0...v6.4.0) (2022-08-10)
 
 

diff --git a/package.json b/package.json
@@ -1,7 +1,7 @@
 {
   "name": "@google-cloud/storage",
   "description": "Cloud Storage Client Library for Node.js",
-  "version": "6.4.0",
+  "version": "6.4.1",
   "license": "Apache-2.0",
   "author": "Google Inc.",
   "engines": {

diff --git a/samples/package.json b/samples/package.json
@@ -17,7 +17,7 @@
   },
   "dependencies": {
     "@google-cloud/pubsub": "^3.0.0",
-    "@google-cloud/storage": "^6.4.0",
+    "@google-cloud/storage": "^6.4.1",
     "node-fetch": "^2.6.7",
     "uuid": "^8.0.0",
     "yargs": "^16.0.0"

diff --git a/samples/system-test/encryption.test.js b/samples/system-test/encryption.test.js
@@ -34,6 +34,7 @@ const kmsKeyName = process.env.GOOGLE_CLOUD_KMS_KEY_US;
 const fileName = 'test.txt';
 const filePath = path.join(__dirname, '../resources', fileName);
 const downloadFilePath = path.join(__dirname, '../resources/downloaded.txt');
+const doesNotExistPrecondition = 0;
 
 const key = crypto.randomBytes(32).toString('base64');
 
@@ -56,7 +57,7 @@ it('should generate a key', () => {
 
 it('should upload a file', async () => {
   const output = execSync(
-    `node uploadEncryptedFile.js ${bucketName} ${filePath} ${fileName} ${key}`
+    `node uploadEncryptedFile.js ${bucketName} ${filePath} ${fileName} ${key} ${doesNotExistPrecondition}`
   );
   assert.match(
     output,

diff --git a/samples/system-test/files.test.js b/samples/system-test/files.test.js
@@ -62,7 +62,7 @@ describe('file', () => {
 
   it('should upload a file', async () => {
     const output = execSync(
-      `node uploadFile.js ${bucketName} ${filePath} ${fileName}`
+      `node uploadFile.js ${bucketName} ${filePath} ${fileName} ${doesNotExistPrecondition}`
     );
     assert.match(output, new RegExp(`${filePath} uploaded to ${bucketName}`));
     const [exists] = await bucket.file(fileName).exists();
@@ -85,7 +85,7 @@ describe('file', () => {
 
   it('should upload a file without authentication', async () => {
     const output = execSync(
-      `node uploadWithoutAuthentication.js ${bucketName} ${fileContents} ${fileName}`
+      `node uploadWithoutAuthentication.js ${bucketName} ${fileContents} ${fileName} ${doesNotExistPrecondition}`
     );
     assert.match(output, new RegExp(`${fileName} uploaded to ${bucketName}`));
     const [exists] = await bucket.file(fileName).exists();
@@ -111,8 +111,9 @@ describe('file', () => {
   });
 
   it('should upload a file with a kms key', async () => {
+    const [metadata] = await bucket.file(fileName).getMetadata();
     const output = execSync(
-      `node uploadFileWithKmsKey.js ${bucketName} ${filePath} ${kmsKeyName}`
+      `node uploadFileWithKmsKey.js ${bucketName} ${filePath} ${kmsKeyName} ${metadata.generation}`
     );
     assert.include(
       output,

diff --git a/samples/uploadEncryptedFile.js b/samples/uploadEncryptedFile.js
@@ -17,7 +17,8 @@ function main(
   bucketName = 'my-bucket',
   filePath = path.join(__dirname, '../resources', 'test.txt'),
   destFileName = 'test.txt',
-  key = process.env.GOOGLE_CLOUD_KMS_KEY_US
+  key = process.env.GOOGLE_CLOUD_KMS_KEY_US,
+  generationMatchPrecondition = 0
 ) {
   // [START storage_upload_encrypted_file]
   /**
@@ -45,6 +46,15 @@ function main(
     const options = {
       destination: destFileName,
       encryptionKey: Buffer.from(key, 'base64'),
+
+      // Optional:
+      // Set a generation-match precondition to avoid potential race conditions
+      // and data corruptions. The request to upload is aborted if the object's
+      // generation number does not match your precondition. For a destination
+      // object that does not yet exist, set the ifGenerationMatch precondition to 0
+      // If the destination object already exists in your bucket, set instead a
+      // generation-match precondition using its generation number.
+      preconditionOpts: {ifGenerationMatch: generationMatchPrecondition},
     };
 
     await storage.bucket(bucketName).upload(filePath, options);

diff --git a/samples/uploadFile.js b/samples/uploadFile.js
@@ -15,7 +15,8 @@
 function main(
   bucketName = 'my-bucket',
   filePath = './local/path/to/file.txt',
-  destFileName = 'file.txt'
+  destFileName = 'file.txt',
+  generationMatchPrecondition = 0
 ) {
   // [START storage_upload_file]
   /**
@@ -37,10 +38,19 @@ function main(
   const storage = new Storage();
 
   async function uploadFile() {
-    await storage.bucket(bucketName).upload(filePath, {
+    const options = {
       destination: destFileName,
-    });
-
+      // Optional:
+      // Set a generation-match precondition to avoid potential race conditions
+      // and data corruptions. The request to upload is aborted if the object's
+      // generation number does not match your precondition. For a destination
+      // object that does not yet exist, set the ifGenerationMatch precondition to 0
+      // If the destination object already exists in your bucket, set instead a
+      // generation-match precondition using its generation number.
+      preconditionOpts: {ifGenerationMatch: generationMatchPrecondition},
+    };
+
+    await storage.bucket(bucketName).upload(filePath, options);
     console.log(`${filePath} uploaded to ${bucketName}`);
   }
 

diff --git a/samples/uploadFileWithKmsKey.js b/samples/uploadFileWithKmsKey.js
@@ -23,7 +23,8 @@
 function main(
   bucketName = 'my-bucket',
   filePath = 'test.txt',
-  kmsKeyName = process.env.GOOGLE_CLOUD_KMS_KEY_US
+  kmsKeyName = process.env.GOOGLE_CLOUD_KMS_KEY_US,
+  generationMatchPrecondition = 0
 ) {
   // [START storage_upload_with_kms_key]
   /**
@@ -45,9 +46,19 @@ function main(
   const storage = new Storage();
 
   async function uploadFileWithKmsKey() {
-    await storage.bucket(bucketName).upload(filePath, {
+    const options = {
       kmsKeyName,
-    });
+      // Optional:
+      // Set a generation-match precondition to avoid potential race conditions
+      // and data corruptions. The request to upload is aborted if the object's
+      // generation number does not match your precondition. For a destination
+      // object that does not yet exist, set the ifGenerationMatch precondition to 0
+      // If the destination object already exists in your bucket, set instead a
+      // generation-match precondition using its generation number.
+      preconditionOpts: {ifGenerationMatch: generationMatchPrecondition},
+    };
+
+    await storage.bucket(bucketName).upload(filePath, options);
 
     console.log(`${filePath} uploaded to ${bucketName} using ${kmsKeyName}.`);
   }

diff --git a/samples/uploadWithoutAuthentication.js b/samples/uploadWithoutAuthentication.js
@@ -15,7 +15,8 @@
 function main(
   bucketName = 'my-bucket',
   contents = 'these are my file contents',
-  destFileName = 'file.txt'
+  destFileName = 'file.txt',
+  generationMatchPrecondition = 0
 ) {
   // [START storage_upload_without_authentication]
   /**
@@ -43,13 +44,24 @@ function main(
     // you can make requests without credentials.
     const [location] = await file.createResumableUpload(); //auth required
 
-    // Passes the location to file.save so you don't need to
-    // authenticate this call
-    await file.save(contents, {
+    const options = {
       uri: location,
       resumable: true,
       validation: false,
-    });
+
+      // Optional:
+      // Set a generation-match precondition to avoid potential race conditions
+      // and data corruptions. The request to upload is aborted if the object's
+      // generation number does not match your precondition. For a destination
+      // object that does not yet exist, set the ifGenerationMatch precondition to 0
+      // If the destination object already exists in your bucket, set instead a
+      // generation-match precondition using its generation number.
+      preconditionOpts: {ifGenerationMatch: generationMatchPrecondition},
+    };
+
+    // Passes the location to file.save so you don't need to
+    // authenticate this call
+    await file.save(contents, options);
 
     console.log(`${destFileName} uploaded to ${bucketName}`);
   }

diff --git a/src/resumable-upload.ts b/src/resumable-upload.ts
@@ -742,9 +742,18 @@ export class Upload extends Writable {
         responseReceived = true;
         this.responseHandler(resp);
       }
-    } catch (err) {
-      const e = err as Error;
-      this.destroy(e);
+    } catch (e) {
+      const err = e as ApiError;
+
+      if (this.retryOptions.retryableErrorFn!(err)) {
+        this.attemptDelayedRetry({
+          status: NaN,
+          data: err,
+        });
+        return;
+      }
+
+      this.destroy(err);
     }
   }
 
@@ -833,7 +842,16 @@ export class Upload extends Writable {
       }
       this.offset = 0;
     } catch (e) {
-      const err = e as GaxiosError;
+      const err = e as ApiError;
+
+      if (this.retryOptions.retryableErrorFn!(err)) {
+        this.attemptDelayedRetry({
+          status: NaN,
+          data: err,
+        });
+        return;
+      }
+
       this.destroy(err);
     }
   }
@@ -899,25 +917,6 @@ export class Upload extends Writable {
     return successfulRequest ? res : null;
   }
 
-  private restart() {
-    if (this.numBytesWritten) {
-      const message =
-        'Attempting to restart an upload after unrecoverable bytes have been written from upstream. Stopping as this could result in data loss. Initiate a new upload to continue.';
-
-      this.emit('error', new RangeError(message));
-      return;
-    }
-
-    this.lastChunkSent = Buffer.alloc(0);
-    this.createURI(err => {
-      if (err) {
-        return this.destroy(err);
-      }
-      this.startUploading();
-      return;
-    });
-  }
-
   /**
    * @return {bool} is the request good?
    */
@@ -941,7 +940,7 @@ export class Upload extends Writable {
   /**
    * @param resp GaxiosResponse object from previous attempt
    */
-  private attemptDelayedRetry(resp: GaxiosResponse) {
+  private attemptDelayedRetry(resp: Pick<GaxiosResponse, 'data' | 'status'>) {
     if (this.numRetries < this.retryOptions.maxRetries!) {
       if (
         resp.status === NOT_FOUND_STATUS_CODE &&

diff --git a/src/storage.ts b/src/storage.ts
@@ -261,11 +261,12 @@ const IDEMPOTENCY_STRATEGY_DEFAULT = IdempotencyStrategy.RetryConditional;
  * @return {boolean} True if the API request should be retried, false otherwise.
  */
 export const RETRYABLE_ERR_FN_DEFAULT = function (err?: ApiError) {
-  const isConnectionProblem = (reason: string | undefined) => {
+  const isConnectionProblem = (reason: string) => {
     return (
-      (reason && reason.includes('eai_again')) || //DNS lookup error
+      reason.includes('eai_again') || // DNS lookup error
       reason === 'econnreset' ||
-      reason === 'unexpected connection closure'
+      reason === 'unexpected connection closure' ||
+      reason === 'epipe'
     );
   };
 
@@ -284,7 +285,7 @@ export const RETRYABLE_ERR_FN_DEFAULT = function (err?: ApiError) {
     if (err.errors) {
       for (const e of err.errors) {
         const reason = e?.reason?.toString().toLowerCase();
-        if (isConnectionProblem(reason)) {
+        if (reason && isConnectionProblem(reason)) {
           return true;
         }
       }

diff --git a/system-test/storage.ts b/system-test/storage.ts
@@ -71,7 +71,9 @@ describe('storage', () => {
   const RETENTION_DURATION_SECONDS = 10;
 
   const storage = new Storage({
-    retryOptions: {idempotencyStrategy: IdempotencyStrategy.RetryAlways},
+    retryOptions: {
+      idempotencyStrategy: IdempotencyStrategy.RetryAlways,
+    },
   });
   const bucket = storage.bucket(generateName());
 
@@ -152,7 +154,10 @@ describe('storage', () => {
       // eslint-disable-next-line @typescript-eslint/no-var-requires
       const {Storage} = require('../src');
       storageWithoutAuth = new Storage({
-        retryOptions: {idempotencyStrategy: IdempotencyStrategy.RetryAlways},
+        retryOptions: {
+          idempotencyStrategy: IdempotencyStrategy.RetryAlways,
+          retryDelayMultiplier: 3,
+        },
       });
     });
 
@@ -219,6 +224,12 @@ describe('storage', () => {
 
   describe('acls', () => {
     describe('buckets', () => {
+      // Some bucket update operations have a rate limit.
+      // Introduce a delay between tests to avoid getting an error.
+      beforeEach(done => {
+        setTimeout(done, 1000);
+      });
+
       it('should get access controls', async () => {
         const accessControls = await bucket.acl.get();
         assert(Array.isArray(accessControls));

diff --git a/test/index.ts b/test/index.ts
@@ -311,6 +311,20 @@ describe('Storage', () => {
       assert.strictEqual(calledWith.retryOptions.retryableErrorFn(error), true);
     });
 
+    it('should retry a broken pipe error', () => {
+      const storage = new Storage({
+        projectId: PROJECT_ID,
+      });
+      const calledWith = storage.calledWith_[0];
+      const error = new ApiError('Broken pipe');
+      error.errors = [
+        {
+          reason: 'EPIPE',
+        },
+      ];
+      assert.strictEqual(calledWith.retryOptions.retryableErrorFn(error), true);
+    });
+
     it('should not retry a 999 error', () => {
       const storage = new Storage({
         projectId: PROJECT_ID,