From 4ab85e0173647a7b24ed9fed0bba642f66e1f6d1 Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Thu, 12 May 2022 18:09:09 +0000 Subject: [PATCH 1/4] refactor(deps): Remove get-stream in favor of async iterators --- package.json | 1 - src/file.ts | 19 +++++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 849d6102a..a10e604ad 100644 --- a/package.json +++ b/package.json @@ -64,7 +64,6 @@ "ent": "^2.2.0", "extend": "^3.0.2", "gaxios": "^4.0.0", - "get-stream": "^6.0.0", "google-auth-library": "^7.14.1", "hash-stream-validation": "^0.2.2", "mime": "^3.0.0", diff --git a/src/file.ts b/src/file.ts index 2157a1fb7..6c5cdafd3 100644 --- a/src/file.ts +++ b/src/file.ts @@ -24,7 +24,6 @@ import { import {promisifyAll} from '@google-cloud/promisify'; import compressible = require('compressible'); -import getStream = require('get-stream'); import * as crypto from 'crypto'; import * as dateFormat from 'date-and-time'; import * as extend from 'extend'; @@ -1394,8 +1393,8 @@ class File extends ServiceObject { ) => { if (err) { // Get error message from the body. - getStream(rawResponseStream).then(body => { - err.message = body; + this.getBufferFromReadable(rawResponseStream).then(body => { + err.message = body.toString('utf-8'); throughStream.destroy(err); }); @@ -2172,10 +2171,9 @@ class File extends ServiceObject { fileStream.pipe(writable).on('error', callback).on('finish', callback); }); } else { - getStream - .buffer(fileStream) + this.getBufferFromReadable(fileStream) .then(contents => callback?.(null, contents)) - .catch(callback as (error: RequestError) => void); + .catch(callback as (err: RequestError) => void); } } @@ -4104,6 +4102,15 @@ class File extends ServiceObject { this.storage.retryOptions.autoRetry = false; } } + + private async getBufferFromReadable(readable: Readable): Promise { + let buf = Buffer.alloc(0); + for await (const chunk of readable) { + buf = Buffer.concat([buf, chunk]); + } + + return buf; + } } /*! Developer Documentation From 5bafb5cf84e62d2b38b68d824315faa1b2d351d6 Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Thu, 12 May 2022 18:53:53 +0000 Subject: [PATCH 2/4] explicitly handle stream error for node < 16 --- src/file.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/file.ts b/src/file.ts index 6c5cdafd3..c03ca8c69 100644 --- a/src/file.ts +++ b/src/file.ts @@ -4104,6 +4104,9 @@ class File extends ServiceObject { } private async getBufferFromReadable(readable: Readable): Promise { + readable.once('error', e => { + throw e; + }); let buf = Buffer.alloc(0); for await (const chunk of readable) { buf = Buffer.concat([buf, chunk]); From f2afb7989f2a9405a938b6a8982a1ac545ed5de0 Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Thu, 12 May 2022 21:58:38 +0000 Subject: [PATCH 3/4] clean up implementation, fix unit tests --- src/file.ts | 12 +++++------- test/file.ts | 17 ++++++++++++++--- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/file.ts b/src/file.ts index c03ca8c69..31d968d86 100644 --- a/src/file.ts +++ b/src/file.ts @@ -1394,7 +1394,7 @@ class File extends ServiceObject { if (err) { // Get error message from the body. this.getBufferFromReadable(rawResponseStream).then(body => { - err.message = body.toString('utf-8'); + err.message = body.toString('utf8'); throughStream.destroy(err); }); @@ -4104,15 +4104,12 @@ class File extends ServiceObject { } private async getBufferFromReadable(readable: Readable): Promise { - readable.once('error', e => { - throw e; - }); - let buf = Buffer.alloc(0); + const buf = []; for await (const chunk of readable) { - buf = Buffer.concat([buf, chunk]); + buf.push(chunk); } - return buf; + return Buffer.concat(buf); } } @@ -4128,6 +4125,7 @@ promisifyAll(File, { 'save', 'setEncryptionKey', 'shouldRetryBasedOnPreconditionAndIdempotencyStrat', + 'getBufferFromReadable', ], }); diff --git a/test/file.ts b/test/file.ts index 3c461a729..132c25de7 100644 --- a/test/file.ts +++ b/test/file.ts @@ -103,6 +103,7 @@ const fakePromisify = { 'save', 'setEncryptionKey', 'shouldRetryBasedOnPreconditionAndIdempotencyStrat', + 'getBufferFromReadable', ]); }, }; @@ -2651,8 +2652,13 @@ describe('File', () => { it('should only execute callback once', done => { Object.assign(fileReadStream, { _read(this: Readable) { - this.emit('error', new Error('Error.')); - this.emit('error', new Error('Error.')); + // Do not fire the errors immediately as this is a synchronous operation here + // and the iterator getter is also synchronous in file.getBufferFromReadable. + // this is only an issue for <= node 12. This cannot happen in practice. + process.nextTick(() => { + this.emit('error', new Error('Error.')); + this.emit('error', new Error('Error.')); + }); }, }); @@ -2685,7 +2691,12 @@ describe('File', () => { Object.assign(fileReadStream, { _read(this: Readable) { - this.emit('error', error); + // Do not fire the errors immediately as this is a synchronous operation here + // and the iterator getter is also synchronous in file.getBufferFromReadable. + // this is only an issue for <= node 12. This cannot happen in practice. + process.nextTick(() => { + this.emit('error', error); + }); }, }); From a00903435944065ed67560460130ff72a4cd1c4a Mon Sep 17 00:00:00 2001 From: Denis DelGrosso Date: Thu, 12 May 2022 22:13:02 +0000 Subject: [PATCH 4/4] update testbench docker image to 0.20.0 --- conformance-test/testBenchUtil.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conformance-test/testBenchUtil.ts b/conformance-test/testBenchUtil.ts index 2a813048c..490d719b7 100644 --- a/conformance-test/testBenchUtil.ts +++ b/conformance-test/testBenchUtil.ts @@ -21,7 +21,7 @@ const PORT = new URL(HOST).port; const CONTAINER_NAME = 'storage-testbench'; const DEFAULT_IMAGE_NAME = 'gcr.io/cloud-devrel-public-resources/storage-testbench'; -const DEFAULT_IMAGE_TAG = 'v0.14.0'; +const DEFAULT_IMAGE_TAG = 'v0.20.0'; const DOCKER_IMAGE = `${DEFAULT_IMAGE_NAME}:${DEFAULT_IMAGE_TAG}`; const PULL_CMD = `docker pull ${DOCKER_IMAGE}`; const RUN_CMD = `docker run --rm -d -p ${PORT}:${PORT} --name ${CONTAINER_NAME} ${DOCKER_IMAGE} && sleep 1`;