From 5deed6c2e1b2e09115323faf6ad4fb1a96dd73ad Mon Sep 17 00:00:00 2001 From: williamlardier Date: Thu, 5 Oct 2023 11:08:07 +0200 Subject: [PATCH 1/3] ARSN-370: fix memory leak The MongoDBReadStreams are not properly destroyed in both the Bucket V1 and V0 cases. In the V1 case, only the pipe-ed stream, the Transform one, is cleaned. In the V0 case, we directly call the callback without properly cleaning the stream, leaving open, in both cases, the mongodb cursors, that in turn affect the mongos memory consumption. --- .../mongoclient/MongoClientInterface.js | 11 ++++++++++ .../metadata/mongodb/listObject.spec.js | 21 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/lib/storage/metadata/mongoclient/MongoClientInterface.js b/lib/storage/metadata/mongoclient/MongoClientInterface.js index 403f708ed..4f39e4983 100644 --- a/lib/storage/metadata/mongoclient/MongoClientInterface.js +++ b/lib/storage/metadata/mongoclient/MongoClientInterface.js @@ -1641,9 +1641,11 @@ class MongoClientInterface { const c = this.getCollection(bucketName); const getLatestVersion = this.getLatestVersion; let stream; + let baseStream; if (!params.secondaryStreamParams) { // listing masters only (DelimiterMaster) stream = new MongoReadStream(c, params.mainStreamParams, params.mongifiedSearch); + baseStream = stream; if (vFormat === BUCKET_VERSIONS.v1) { /** * When listing masters only in v1 we can't just skip PHD @@ -1685,6 +1687,11 @@ class MongoClientInterface { }, }); stream = stream.pipe(resolvePhdKey); + // Propagate the 'end' event from resolvePhdKey to stream + // to properly cleanup resources. + resolvePhdKey.on('end', () => { + baseStream.emit('end'); + }); } } else { // listing both master and version keys (delimiterVersion Algo) @@ -1736,10 +1743,14 @@ class MongoClientInterface { }; log.error( 'internalListObjectV1: error listing objects', logObj); + // call explicitly the destroy method to clean the mongodb cursor + stream.destroy(); cbOnce(err); }) .on('end', () => { const data = extension.result(); + // call explicitly the destroy method to clean the mongodb cursor + stream.destroy(); cbOnce(null, data); }); return undefined; diff --git a/tests/functional/metadata/mongodb/listObject.spec.js b/tests/functional/metadata/mongodb/listObject.spec.js index 6dcaa3df1..a387b7214 100644 --- a/tests/functional/metadata/mongodb/listObject.spec.js +++ b/tests/functional/metadata/mongodb/listObject.spec.js @@ -8,6 +8,8 @@ const MetadataWrapper = require('../../../../lib/storage/metadata/MetadataWrapper'); const { versioning } = require('../../../../index'); const { BucketVersioningKeyFormat } = versioning.VersioningConstants; +const sinon = require('sinon'); +const MongoReadStream = require('../../../../lib/storage/metadata/mongoclient/readStream'); const IMPL_NAME = 'mongodb'; const DB_NAME = 'metadata'; @@ -477,6 +479,25 @@ describe('MongoClientInterface::metadata.listObject', () => { }), ], done); }); + + it('Should properly destroy the MongoDBReadStream', done => { + // eslint-disable-next-line func-names + const destroyStub = sinon.stub(MongoReadStream.prototype, 'destroy').callsFake(function (...args) { + // You can add extra logic here if needed + MongoReadStream.prototype.destroy.wrappedMethod.apply(this, ...args); + }); + const params = { + listingType: 'DelimiterMaster', + maxKeys: 100, + }; + return metadata.listObject(BUCKET_NAME, params, logger, err => { + assert.ifError(err); + assert(destroyStub.called, 'Destroy should have been called on MongoReadStream'); + // Restore original destroy method + destroyStub.restore(); + return done(); + }); + }); }); }); }); From 4aa8b5cc6e82d4f69773e5d3cedcc60b9a22f39c Mon Sep 17 00:00:00 2001 From: williamlardier Date: Fri, 6 Oct 2023 09:06:01 +0200 Subject: [PATCH 2/3] ARSN-370: handle error cases --- .../mongoclient/MongoClientInterface.js | 13 +++++++++- .../metadata/mongodb/listObject.spec.js | 25 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/lib/storage/metadata/mongoclient/MongoClientInterface.js b/lib/storage/metadata/mongoclient/MongoClientInterface.js index 4f39e4983..8bf6cf10c 100644 --- a/lib/storage/metadata/mongoclient/MongoClientInterface.js +++ b/lib/storage/metadata/mongoclient/MongoClientInterface.js @@ -1640,6 +1640,7 @@ class MongoClientInterface { internalListObject(bucketName, params, extension, vFormat, log, cb) { const c = this.getCollection(bucketName); const getLatestVersion = this.getLatestVersion; + const cbOnce = jsutil.once(cb); let stream; let baseStream; if (!params.secondaryStreamParams) { @@ -1692,6 +1693,17 @@ class MongoClientInterface { resolvePhdKey.on('end', () => { baseStream.emit('end'); }); + baseStream.on('error', err => { + const logObj = { + rawError: err, + error: err.message, + errorStack: err.stack, + }; + log.error( + 'internalListObjectV1: error listing objects', logObj); + baseStream.destroy(); + return cbOnce(err); + }); } } else { // listing both master and version keys (delimiterVersion Algo) @@ -1706,7 +1718,6 @@ class MongoClientInterface { extension, gte: gteParams, }); - const cbOnce = jsutil.once(cb); skip.setListingEndCb(() => { stream.emit('end'); stream.destroy(); diff --git a/tests/functional/metadata/mongodb/listObject.spec.js b/tests/functional/metadata/mongodb/listObject.spec.js index a387b7214..676e82001 100644 --- a/tests/functional/metadata/mongodb/listObject.spec.js +++ b/tests/functional/metadata/mongodb/listObject.spec.js @@ -498,6 +498,31 @@ describe('MongoClientInterface::metadata.listObject', () => { return done(); }); }); + + + it('Should properly destroy the MongoDBReadStream on error', done => { + // eslint-disable-next-line func-names + const destroyStub = sinon.stub(MongoReadStream.prototype, 'destroy').callsFake(function (...args) { + // You can add extra logic here if needed + MongoReadStream.prototype.destroy.wrappedMethod.apply(this, ...args); + }); + // stub the cursor creation to emit an error + // eslint-disable-next-line func-names + const readStub = sinon.stub(MongoReadStream.prototype, '_read').callsFake(function () { + this.emit('error', new Error('error')); + }); + const params = { + listingType: 'DelimiterMaster', + maxKeys: 100, + }; + return metadata.listObject(BUCKET_NAME, params, logger, err => { + assert(err, 'Expected an error'); + assert(destroyStub.called, 'Destroy should have been called on MongoReadStream'); + destroyStub.restore(); + readStub.restore(); + return done(); + }); + }); }); }); }); From 17b5bbc233b0bfbca8b68a45dcd730903479a39e Mon Sep 17 00:00:00 2001 From: williamlardier Date: Fri, 6 Oct 2023 09:14:13 +0200 Subject: [PATCH 3/3] ARSN-370: bump project version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index a58c587ca..ce46184df 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "engines": { "node": ">=16" }, - "version": "8.1.112", + "version": "8.1.113", "description": "Common utilities for the S3 project components", "main": "build/index.js", "repository": {