From ecd7bd600c8735ffd946ebadd5bd2a82734a4168 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Fri, 17 Dec 2021 14:08:37 +0100 Subject: [PATCH] [INTERNAL] Move resource content into separate instance This should make resource clone operations cheaper since the content doesn't need to be read and cloned immediately --- lib/Resource.js | 189 +++++---------------- lib/ResourceContent.js | 232 ++++++++++++++++++++++++++ test/lib/Resource.js | 24 +-- test/lib/adapters/FileSystem_write.js | 2 +- 4 files changed, 287 insertions(+), 160 deletions(-) create mode 100644 lib/ResourceContent.js diff --git a/lib/Resource.js b/lib/Resource.js index ecdd96f8..fb0c0923 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -1,6 +1,6 @@ -const stream = require("stream"); const clone = require("clone"); -const path = require("path"); +const nodePath = require("path"); +const ResourceContent = require("./ResourceContent"); const fnTrue = () => true; const fnFalse = () => false; @@ -38,16 +38,19 @@ class Resource { * stream of the content of this resource (cannot be used in conjunction with parameters buffer, * string or stream). * In some cases this is the most memory-efficient way to supply resource content + * @param {module:@ui5/fs.ResourceContent} [parameters.contentInstance] Resource content instance * @param {object} [parameters.project] Experimental, internal parameter. Do not use */ - constructor({path, statInfo, buffer, string, createStream, stream, project}) { + constructor({path, statInfo, buffer, string, createStream, stream, contentInstance, project}) { if (!path) { throw new Error("Cannot create Resource: path parameter missing"); } - if (buffer && createStream || buffer && string || string && createStream || buffer && stream || - string && stream || createStream && stream) { + if (buffer && createStream || buffer && string || buffer && stream || buffer && contentInstance || + string && createStream || string && stream || string && contentInstance || + stream && createStream || stream && contentInstance || + contentInstance && createStream) { throw new Error("Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream or createStream"); + "Buffer, string, stream, createStream or contentInstance"); } this._path = path; @@ -71,14 +74,14 @@ class Resource { birthtime: new Date() }; - if (createStream) { - this._createStream = createStream; - } else if (stream) { - this._stream = stream; - } else if (buffer) { - this.setBuffer(buffer); - } else if (typeof string === "string" || string instanceof String) { - this.setString(string); + if (contentInstance) { + this._content = contentInstance; + } else { + try { + this._content = new ResourceContent({buffer, string, createStream, stream}); + } catch (err) { + throw new Error(`Failed to create content for resource ${this._path}: ${err.message}`); + } } // Tracing: @@ -92,17 +95,10 @@ class Resource { * @returns {Promise} Promise resolving with a buffer of the resource content. */ async getBuffer() { - if (this._contentDrained) { - throw new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - if (this._buffer) { - return this._buffer; - } else if (this._createStream || this._stream) { - return this._getBufferFromStream(); - } else { - throw new Error(`Resource ${this._path} has no content`); + try { + return await this._content.getBuffer(); + } catch (err) { + throw new Error(`Failed to get buffer for resource ${this._path}: ${err.message}`); } } @@ -113,14 +109,7 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { - this._createStream = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } - this._stream = null; - this._buffer = buffer; - this._contentDrained = false; - this._streamDrained = false; + this._content = new ResourceContent({buffer}); } /** @@ -129,13 +118,12 @@ class Resource { * @public * @returns {Promise} Promise resolving with the resource content. */ - getString() { - if (this._contentDrained) { - return Promise.reject(new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set.")); + async getString() { + try { + return await this._content.getString(); + } catch (err) { + throw new Error(`Failed to get string for resource ${this._path}: ${err.message}`); } - return this.getBuffer().then((buffer) => buffer.toString()); } /** @@ -145,7 +133,7 @@ class Resource { * @param {string} string Resource content */ setString(string) { - this.setBuffer(Buffer.from(string, "utf8")); + this._content = new ResourceContent({string}); } /** @@ -160,32 +148,11 @@ class Resource { * @returns {stream.Readable} Readable stream for the resource content. */ getStream() { - if (this._contentDrained) { - throw new Error(`Content of Resource ${this._path} has been drained. ` + - "This might be caused by requesting resource content after a content stream has been " + - "requested and no new content (e.g. a new stream) has been set."); - } - let contentStream; - if (this._buffer) { - const bufferStream = new stream.PassThrough(); - bufferStream.end(this._buffer); - contentStream = bufferStream; - } else if (this._createStream || this._stream) { - contentStream = this._getStream(); - } - if (!contentStream) { - throw new Error(`Resource ${this._path} has no content`); + try { + return this._content.getStream(); + } catch (err) { + throw new Error(`Failed to get stream for resource ${this._path}: ${err.message}`); } - // If a stream instance is being returned, it will typically get drained be the consumer. - // In that case, further content access will result in a "Content stream has been drained" error. - // However, depending on the execution environment, a resources content stream might have been - // transformed into a buffer. In that case further content access is possible as a buffer can't be - // drained. - // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag - // the resource content as "drained" every time a stream is requested. Even if actually a buffer or - // createStream callback is being used. - this._contentDrained = true; - return contentStream; } /** @@ -196,19 +163,13 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { - this._buffer = null; - // if (this._stream) { // TODO this may cause strange issues - // this._stream.destroy(); - // } + const options = {}; if (typeof stream === "function") { - this._createStream = stream; - this._stream = null; + options.createStream = stream; } else { - this._stream = stream; - this._createStream = null; + options.stream = stream; } - this._contentDrained = false; - this._streamDrained = false; + this._content = new ResourceContent(options); } /** @@ -253,16 +214,11 @@ class Resource { * @returns {Promise} size in bytes, 0 if there is no content yet */ async getSize() { - // if resource does not have any content it should have 0 bytes - if (!this._buffer && !this._createStream && !this._stream) { - return 0; - } - const buffer = await this.getBuffer(); - return buffer.byteLength; + return this._content.getSize(); } _getNameFromPath(virPath) { - return path.posix.basename(virPath); + return nodePath.posix.basename(virPath); } /** @@ -280,30 +236,14 @@ class Resource { * @public * @returns {Promise} Promise resolving with the clone */ - clone() { + async clone() { const options = { path: this._path, - statInfo: clone(this._statInfo) - }; - - const addContentOption = () => { - if (this._stream) { - return this._getBufferFromStream().then(function(buffer) { - options.buffer = buffer; - }); - } else { - if (this._createStream) { - options.createStream = this._createStream; - } else if (this._buffer) { - options.buffer = this._buffer; - } - return Promise.resolve(); - } + statInfo: clone(this._statInfo), + contentInstance: this._content }; - return addContentOption().then(() => { - return new Resource(options); - }); + return new Resource(options); } /** @@ -322,51 +262,6 @@ class Resource { return tree; } - - /** - * Returns the content as stream. - * - * @private - * @returns {stream.Readable} Readable stream - */ - _getStream() { - if (this._streamDrained) { - throw new Error(`Content stream of Resource ${this._path} is flagged as drained.`); - } - if (this._createStream) { - return this._createStream(); - } - this._streamDrained = true; - return this._stream; - } - - /** - * Converts the buffer into a stream. - * - * @private - * @returns {Promise} Promise resolving with buffer. - */ - _getBufferFromStream() { - if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream - return this._buffering; - } - return this._buffering = new Promise((resolve, reject) => { - const contentStream = this._getStream(); - const buffers = []; - contentStream.on("data", (data) => { - buffers.push(data); - }); - contentStream.on("error", (err) => { - reject(err); - }); - contentStream.on("end", () => { - const buffer = Buffer.concat(buffers); - this.setBuffer(buffer); - this._buffering = null; - resolve(buffer); - }); - }); - } } module.exports = Resource; diff --git a/lib/ResourceContent.js b/lib/ResourceContent.js new file mode 100644 index 00000000..584be4f7 --- /dev/null +++ b/lib/ResourceContent.js @@ -0,0 +1,232 @@ +const stream = require("stream"); + +/** + * Content of a resource + * + * @private + * @memberof module:@ui5/fs + */ +class ResourceContent { + /** + * + * + * @public + * @param {object} parameters Parameters + * [fs.Stats]{@link https://nodejs.org/api/fs.html#fs_class_fs_stats} or similar object + * @param {Buffer} [parameters.buffer] Content of this resources as a Buffer instance + * (cannot be used in conjunction with parameters string, stream or createStream) + * @param {string} [parameters.string] Content of this resources as a string + * (cannot be used in conjunction with parameters buffer, stream or createStream) + * @param {Stream} [parameters.stream] Readable stream of the content of this resource + * (cannot be used in conjunction with parameters buffer, string or createStream) + * @param {module:@ui5/fs.Resource~createStream} [parameters.createStream] Function callback that returns a readable + * stream of the content of this resource (cannot be used in conjunction with parameters buffer, + * string or stream). + * In some cases this is the most memory-efficient way to supply resource content + */ + constructor({buffer, string, createStream, stream}) { + if (buffer && createStream || buffer && string || string && createStream || buffer && stream || + string && stream || createStream && stream) { + throw new Error("Cannot create ResourceContent: Please set only one content parameter. " + + "Buffer, string, stream or createStream"); + } + + if (createStream) { + this._createStream = createStream; + } else if (stream) { + this._stream = stream; + } else if (buffer) { + this._setBuffer(buffer); + } else if (typeof string === "string" || string instanceof String) { + this._setString(string); + } + } + + /** + * Gets a buffer with the resource content. + * + * @public + * @returns {Promise} Promise resolving with a buffer of the resource content. + */ + async getBuffer() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + if (this._buffer) { + return this._buffer; + } else if (this._createStream || this._stream) { + return this._getBufferFromStream(); + } else { + throw new Error(`No content available`); + } + } + + /** + * Sets a Buffer as content. + * + * @public + * @param {Buffer} buffer Buffer instance + */ + _setBuffer(buffer) { + this._createStream = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } + this._stream = null; + this._buffer = buffer; + this._contentDrained = false; + this._streamDrained = false; + } + + /** + * Gets a string with the resource content. + * + * @public + * @returns {Promise} Promise resolving with the resource content. + */ + async getString() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + return this.getBuffer().then((buffer) => buffer.toString()); + } + + /** + * Sets a String as content + * + * @public + * @param {string} string Resource content + */ + _setString(string) { + this._setBuffer(Buffer.from(string, "utf8")); + } + + /** + * Gets a readable stream for the resource content. + * + * Repetitive calls of this function are only possible if new content has been set in the meantime (through + * [setStream]{@link module:@ui5/fs.Resource#setStream}, [setBuffer]{@link module:@ui5/fs.Resource#setBuffer} + * or [setString]{@link module:@ui5/fs.Resource#setString}). This + * is to prevent consumers from accessing drained streams. + * + * @public + * @returns {stream.Readable} Readable stream for the resource content. + */ + getStream() { + if (this._contentDrained) { + throw new Error(`Content stream has been drained. ` + + "This might be caused by requesting resource content after a content stream has been " + + "requested and no new content (e.g. a new stream) has been set."); + } + let contentStream; + if (this._buffer) { + const bufferStream = new stream.PassThrough(); + bufferStream.end(this._buffer); + contentStream = bufferStream; + } else if (this._createStream || this._stream) { + contentStream = this._getStream(); + } + if (!contentStream) { + throw new Error(`No content available`); + } + // If a stream instance is being returned, it will typically get drained by the consumer. + // In that case, further content access will result in a "Content stream has been drained" error. + // However, depending on the execution environment, a resources content stream might have been + // transformed into a buffer. In that case further content access is possible as a buffer can't be + // drained. + // To prevent unexpected "Content stream has been drained" errors caused by changing environments, we flag + // the resource content as "drained" every time a stream is requested. Even if actually a buffer or + // createStream callback is being used. + this._contentDrained = true; + return contentStream; + } + + /** + * Sets a readable stream as content. + * + * @public + * @param {stream.Readable|module:@ui5/fs.Resource~createStream} stream Readable stream of the resource content or + callback for dynamic creation of a readable stream + */ + _setStream(stream) { + this._buffer = null; + // if (this._stream) { // TODO this may cause strange issues + // this._stream.destroy(); + // } + if (typeof stream === "function") { + this._createStream = stream; + this._stream = null; + } else { + this._stream = stream; + this._createStream = null; + } + this._contentDrained = false; + this._streamDrained = false; + } + + /** + * Size in bytes allocated by the underlying buffer. + * + * @see {TypedArray#byteLength} + * @returns {Promise} size in bytes, 0 if there is no content yet + */ + async getSize() { + // if resource does not have any content it should have 0 bytes + if (!this._buffer && !this._createStream && !this._stream) { + return 0; + } + const buffer = await this.getBuffer(); + return buffer.byteLength; + } + + /** + * Returns the content as stream. + * + * @private + * @returns {stream.Readable} Readable stream + */ + _getStream() { + if (this._streamDrained) { + throw new Error(`Content stream is flagged as drained.`); + } + if (this._createStream) { + return this._createStream(); + } + this._streamDrained = true; + return this._stream; + } + + /** + * Converts the buffer into a stream. + * + * @private + * @returns {Promise} Promise resolving with buffer. + */ + _getBufferFromStream() { + if (this._buffering) { // Prevent simultaneous buffering, causing unexpected access to drained stream + return this._buffering; + } + return this._buffering = new Promise((resolve, reject) => { + const contentStream = this._getStream(); + const buffers = []; + contentStream.on("data", (data) => { + buffers.push(data); + }); + contentStream.on("error", (err) => { + reject(err); + }); + contentStream.on("end", () => { + const buffer = Buffer.concat(buffers); + this._setBuffer(buffer); + this._buffering = null; + resolve(buffer); + }); + }); + } +} + +module.exports = ResourceContent; diff --git a/test/lib/Resource.js b/test/lib/Resource.js index f61c5d52..d90d6dea 100644 --- a/test/lib/Resource.js +++ b/test/lib/Resource.js @@ -60,7 +60,7 @@ test("Resource: constructor with duplicated content parameter", (t) => { }, { instanceOf: Error, message: "Cannot create Resource: Please set only one content parameter. " + - "Buffer, string, stream or createStream" + "Buffer, string, stream, createStream or contentInstance" }); }); @@ -72,7 +72,7 @@ test("Resource: getBuffer with throwing an error", (t) => { }); return resource.getBuffer().catch(function(error) { - t.is(error.message, "Resource my/path/to/resource has no content", + t.is(error.message, "Failed to get buffer for resource my/path/to/resource: No content available", "getBuffer called w/o having a resource content provided"); }); }); @@ -126,7 +126,7 @@ test("Resource: getStream throwing an error", (t) => { resource.getStream(); }, { instanceOf: Error, - message: "Resource my/path/to/resource has no content" + message: "Failed to get stream for resource my/path/to/resource: No content available" }); }); @@ -267,9 +267,9 @@ test("getStream with createStream callback content: Subsequent content requests resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getStream with Buffer content: Subsequent content requests should throw error due to drained " + @@ -279,9 +279,9 @@ test("getStream with Buffer content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getStream with Stream content: Subsequent content requests should throw error due to drained " + @@ -301,9 +301,9 @@ test("getStream with Stream content: Subsequent content requests should throw er resource.getStream(); t.throws(() => { resource.getStream(); - }, {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getBuffer(), {message: /Content of Resource \/app\/index.html has been drained/}); - await t.throwsAsync(resource.getString(), {message: /Content of Resource \/app\/index.html has been drained/}); + }, {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getBuffer(), {message: /Content stream has been drained/}); + await t.throwsAsync(resource.getString(), {message: /Content stream has been drained/}); }); test("getBuffer from Stream content: Subsequent content requests should not throw error due to drained " + diff --git a/test/lib/adapters/FileSystem_write.js b/test/lib/adapters/FileSystem_write.js index 3c15be84..a69f86fc 100644 --- a/test/lib/adapters/FileSystem_write.js +++ b/test/lib/adapters/FileSystem_write.js @@ -82,7 +82,7 @@ test("Write resource in drain mode", async (t) => { assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); }); await t.throwsAsync(resource.getBuffer(), - {message: /Content of Resource \/app\/index.html has been drained/}); + {message: /Failed to get buffer for resource \/app\/index.html: Content stream has been drained/}); }); test("Writing with readOnly and drain options set should fail", async (t) => {