Skip to content

Commit 0ac5f2e

Browse files
committed
fix(storage): destroy local read stream on upload write failure to prevent resource leaks
1 parent 4785152 commit 0ac5f2e

2 files changed

Lines changed: 52 additions & 3 deletions

File tree

handwritten/storage/src/bucket.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4501,17 +4501,22 @@ class Bucket extends ServiceObject<Bucket, BucketMetadata> {
45014501
) {
45024502
newFile.storage.retryOptions.autoRetry = false;
45034503
}
4504+
const readStream = fs.createReadStream(pathString);
45044505
const writable = newFile.createWriteStream(options);
45054506
if (options.onUploadProgress) {
45064507
writable.on('progress', options.onUploadProgress);
45074508
}
4508-
fs.createReadStream(pathString)
4509-
.on('error', bail)
4509+
readStream
4510+
.on('error', err => {
4511+
readStream.destroy();
4512+
bail(err);
4513+
})
45104514
.pipe(writable)
45114515
.on('error', err => {
4516+
readStream.destroy();
45124517
if (
45134518
this.storage.retryOptions.autoRetry &&
4514-
this.storage.retryOptions.retryableErrorFn!(err)
4519+
this.storage.retryOptions.retryableErrorFn!(err as any)
45154520
) {
45164521
return reject(err);
45174522
} else {

handwritten/storage/test/bucket.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,11 +100,18 @@ class FakeNotification {
100100
}
101101

102102
let fsStatOverride: Function | null;
103+
let fsCreateReadStreamOverride: Function | null;
103104
const fakeFs = {
104105
...fs,
105106
stat: (filePath: string, callback: Function) => {
106107
return (fsStatOverride || fs.stat)(filePath, callback);
107108
},
109+
createReadStream: (filePath: string, options?: any) => {
110+
return (fsCreateReadStreamOverride || fs.createReadStream)(
111+
filePath,
112+
options
113+
);
114+
},
108115
};
109116

110117
let pLimitOverride: Function | null;
@@ -3231,6 +3238,43 @@ describe('Bucket', () => {
32313238
});
32323239
});
32333240

3241+
it('should destroy the local read stream if write stream fails', done => {
3242+
const fakeFile = new FakeFile(bucket, 'file-name');
3243+
const options = {destination: fakeFile, resumable: false};
3244+
const originalCreateReadStream = fs.createReadStream;
3245+
let readStream: fs.ReadStream;
3246+
fsCreateReadStreamOverride = (path: string, opts: any) => {
3247+
readStream = originalCreateReadStream(path, opts);
3248+
return readStream;
3249+
};
3250+
3251+
fakeFile.createWriteStream = (options_: CreateWriteStreamOptions) => {
3252+
const ws = new stream.Writable({
3253+
write(chunk, encoding, callback) {
3254+
callback(new Error('write error'));
3255+
},
3256+
});
3257+
return ws;
3258+
};
3259+
3260+
const textfilepath = path.join(
3261+
getDirName(),
3262+
'../../../test/testdata/textfile.txt'
3263+
);
3264+
3265+
bucket.upload(textfilepath, options, (err: Error) => {
3266+
try {
3267+
assert.strictEqual(err.message, 'write error');
3268+
assert.ok(readStream.destroyed);
3269+
done();
3270+
} catch (e) {
3271+
done(e);
3272+
} finally {
3273+
fsCreateReadStreamOverride = null;
3274+
}
3275+
});
3276+
});
3277+
32343278
it('should allow overriding content type', done => {
32353279
const fakeFile = new FakeFile(bucket, 'file-name');
32363280
const metadata = {contentType: 'made-up-content-type'};

0 commit comments

Comments
 (0)