From 7b47ca513306a75d0bd85432aea27fa9d90021a8 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 6 Sep 2023 11:04:03 -0700 Subject: [PATCH 1/5] fix: File#save iterable fixes --- src/file.ts | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/file.ts b/src/file.ts index 6eed2cabd..d5be7488d 100644 --- a/src/file.ts +++ b/src/file.ts @@ -463,7 +463,6 @@ export enum FileExceptionMessages { UPLOAD_MISMATCH = `The uploaded data did not match the data from the server. As a precaution, the file has been deleted. To be sure the content is the same, you should try uploading the file again.`, - STREAM_NOT_READABLE = 'Stream must be readable.', } /** @@ -3644,20 +3643,6 @@ class File extends ServiceObject { } const returnValue = retry( async (bail: (err: Error) => void) => { - if (data instanceof Readable) { - // Make sure any pending async readable operations are finished before - // attempting to check if the stream is readable. - await new Promise(resolve => setImmediate(resolve)); - - if (!data.readable || data.destroyed) { - // Calling pipeline() with a non-readable stream will result in the - // callback being called without an error, and no piping taking - // place. In that case, file.save() would appear to succeed, but - // nothing would be uploaded. - return bail(new Error(FileExceptionMessages.STREAM_NOT_READABLE)); - } - } - return new Promise((resolve, reject) => { if (maxRetries === 0) { this.storage.retryOptions.autoRetry = false; @@ -3673,10 +3658,10 @@ class File extends ServiceObject { !this.storage.retryOptions.autoRetry || !this.storage.retryOptions.retryableErrorFn!(err) ) { - bail(err); + return reject(err); } - reject(err); + return bail(err); }; if (typeof data === 'string' || Buffer.isBuffer(data)) { From 08dfc51b29af8601f90c78b26a7a396eef41c419 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 6 Sep 2023 11:13:07 -0700 Subject: [PATCH 2/5] fix: if logic --- src/file.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/file.ts b/src/file.ts index d5be7488d..32b0d5f64 100644 --- a/src/file.ts +++ b/src/file.ts @@ -3655,8 +3655,8 @@ class File extends ServiceObject { const handleError = (err: Error) => { if ( - !this.storage.retryOptions.autoRetry || - !this.storage.retryOptions.retryableErrorFn!(err) + this.storage.retryOptions.autoRetry && + this.storage.retryOptions.retryableErrorFn!(err) ) { return reject(err); } From 18973aa0601c564f7769600846cd4fa26f47c154 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 6 Sep 2023 11:20:55 -0700 Subject: [PATCH 3/5] test: remove unneccesary test --- test/file.ts | 52 ---------------------------------------------------- 1 file changed, 52 deletions(-) diff --git a/test/file.ts b/test/file.ts index 567cf77d4..4aa19968d 100644 --- a/test/file.ts +++ b/test/file.ts @@ -4365,31 +4365,6 @@ describe('File', () => { } }); - it('Destroyed Readable upload should throw', async () => { - const options = {resumable: false}; - - file.createWriteStream = () => { - throw new Error('unreachable'); - }; - try { - const readable = new Readable({ - read() { - this.push(DATA); - this.push(null); - }, - }); - - readable.destroy(); - - await file.save(readable, options); - } catch (e) { - assert.strictEqual( - (e as Error).message, - FileExceptionMessages.STREAM_NOT_READABLE - ); - } - }); - it('should save a generator with no error', done => { const options = {resumable: false}; file.createWriteStream = () => { @@ -4439,33 +4414,6 @@ describe('File', () => { }); }); - it('should error on invalid async iterator data', done => { - const options = {resumable: false}; - file.createWriteStream = () => { - const writeStream = new PassThrough(); - let errorCalled = false; - writeStream.on('error', () => { - errorCalled = true; - }); - writeStream.on('finish', () => { - assert.ok(errorCalled); - }); - return writeStream; - }; - - const generator = async function* () { - yield {thisIsNot: 'a buffer or a string'}; - }; - - file.save(generator(), options, (err: Error) => { - assert.strictEqual( - err.message, - 'The "chunk" argument must be of type string or an instance of Buffer or Uint8Array. Received an instance of Object' - ); - done(); - }); - }); - it('buffer upload should retry on first failure', async () => { const options = { resumable: false, From 4999bee671d019d94b9cc71840db0d6dc8c2f6bf Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 6 Sep 2023 11:57:08 -0700 Subject: [PATCH 4/5] fix: return on save-stream-bail --- src/file.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/file.ts b/src/file.ts index 32b0d5f64..2de0ebe2e 100644 --- a/src/file.ts +++ b/src/file.ts @@ -3682,7 +3682,7 @@ class File extends ServiceObject { if (typeof data !== 'function') { // Only PipelineSourceFunction can be retried. Async-iterables // and Readable streams can only be consumed once. - bail(err); + return bail(err); } handleError(err); From 7939afd728fe4b2a1accdcafa04c9062b8604aa8 Mon Sep 17 00:00:00 2001 From: Daniel Bankhead Date: Wed, 6 Sep 2023 12:15:43 -0700 Subject: [PATCH 5/5] refactor: remove non-reachable case --- src/file.ts | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/file.ts b/src/file.ts index 2de0ebe2e..c22400282 100644 --- a/src/file.ts +++ b/src/file.ts @@ -3672,13 +3672,6 @@ class File extends ServiceObject { } else { pipeline(data, writable, err => { if (err) { - // If data is not a valid PipelineSource, then pipeline will - // fail without destroying the writable stream. If data is a - // PipelineSource that yields invalid chunks (e.g. a stream in - // object mode or an iterable that does not yield Buffers or - // strings), then pipeline will destroy the writable stream. - if (!writable.destroyed) writable.destroy(); - if (typeof data !== 'function') { // Only PipelineSourceFunction can be retried. Async-iterables // and Readable streams can only be consumed once.