From 3a7c6a7f109443eb83fd45d7c762220f4ad821b1 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 10 Jul 2018 15:05:15 +0200 Subject: [PATCH] [FIX] Prevent FS write from draining Resources content Previously, when writing to an FS-Adapter, a Resources content stream would be drained, making it impossible to access a Resources content again. With this change, a Resources content will be buffered while draining its content stream. An optional 'final' parameter can be supplied to skip buffering to lower the memory demand of the operation. Also add a pessimistic check for drained content streams. --- lib/AbstractReaderWriter.js | 12 +++++++++--- lib/Resource.js | 11 +++++++++-- lib/adapters/FileSystem.js | 33 ++++++++++++++++++++++++++++----- test/lib/resources.js | 12 ++++++------ 4 files changed, 52 insertions(+), 16 deletions(-) diff --git a/lib/AbstractReaderWriter.js b/lib/AbstractReaderWriter.js index 98ec1e73..88495d51 100644 --- a/lib/AbstractReaderWriter.js +++ b/lib/AbstractReaderWriter.js @@ -21,10 +21,13 @@ class AbstractReaderWriter extends AbstractReader { * Writes the content of a resource to a path. * * @param {Resource} resource The Resource to write + * @param {Object} [options] + * @param {boolean} [options.final=false] Keep set to false if the resource should still have + access to its content after the write process * @returns {Promise} Promise resolving once data has been written */ - write(resource) { - return this._write(resource); + write(resource, options = {final: false}) { + return this._write(resource, options); } /** @@ -33,9 +36,12 @@ class AbstractReaderWriter extends AbstractReader { * @abstract * @protected * @param {Resource} resource The Resource to write + * @param {Object} [options] + * @param {boolean} [options.final] Set to false if the resource should still have + access to its content after the write process * @returns {Promise} Promise resolving once data has been written */ - _write(resource) { + _write(resource, options) { throw new Error("Not implemented"); } } diff --git a/lib/Resource.js b/lib/Resource.js index e2dda785..38af5fa5 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -95,6 +95,7 @@ class Resource { // } this._stream = null; this._buffer = buffer; + this._contentDrained = false; } /** @@ -144,6 +145,7 @@ class Resource { // this._stream.destroy(); // } this._stream = stream; + this._contentDrained = false; } /** @@ -245,6 +247,11 @@ class Resource { if (this._createStream) { return this._createStream(); } + if (this._contentDrained) { + throw new Error(`Content stream of Resource ${this._path} has been drained. ` + + "This might be caused by accessing an already (finally) written resource."); + } + this._contentDrained = true; return this._stream; } @@ -256,8 +263,8 @@ class Resource { */ _getBufferFromStream() { return new Promise((resolve, reject) => { - let contentStream = this._getStream(); - let buffers = []; + const contentStream = this._getStream(); + const buffers = []; contentStream.on("data", (data) => { buffers.push(data); }); diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index fa46f168..003f6de7 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -5,6 +5,7 @@ const glob = require("globby"); const defaults = require("defaults"); const clone = require("clone"); const makeDir = require("make-dir"); +const {PassThrough} = require("stream"); const Resource = require("../Resource"); const AbstractAdapter = require("./AbstractAdapter"); @@ -167,9 +168,11 @@ class FileSystem extends AbstractAdapter { * * @private * @param {Resource} resource The Resource + * @param {Object} options + * @param {boolean} options.final Whether this resource can loose access to its content in the write process * @returns {Promise} Promise resolving once data has been written */ - _write(resource) { + _write(resource, {final}) { const relPath = resource.getPath().substr(this._virBasePath.length); const fsPath = path.join(this._fsBasePath, relPath); const dirPath = path.dirname(fsPath); @@ -180,10 +183,30 @@ class FileSystem extends AbstractAdapter { fs }).then(() => { return new Promise((resolve, reject) => { - let contentStream = resource.getStream(); - contentStream.on("error", function(err) { - reject(err); - }); + let contentStream; + if (final) { + contentStream = resource.getStream(); + + contentStream.on("error", function(err) { + reject(err); + }); + } else { + contentStream = new PassThrough(); + const buffers = []; + contentStream.on("error", (err) => { + reject(err); + }); + contentStream.on("data", (data) => { + buffers.push(data); + }); + contentStream.on("end", () => { + let buffer = Buffer.concat(buffers); + resource.setBuffer(buffer); + }); + resource.getStream().pipe(contentStream); + } + + let write = fs.createWriteStream(fsPath); write.on("error", function(err) { reject(err); diff --git a/test/lib/resources.js b/test/lib/resources.js index 91bd650f..51b826e2 100644 --- a/test/lib/resources.js +++ b/test/lib/resources.js @@ -40,7 +40,7 @@ test("Get resource from application.a (/index.html) and write it to /dest/ using })); }); -test("FS RL: GLOB resources from application.a w/ virtual base path prefix", (t) => { +test("FS-Adapter: GLOB resources from application.a w/ virtual base path prefix", (t) => { let readerWriters = t.context.readerWriters; // Get resource from one readerWriter return t.notThrows(readerWriters.source.byGlob("/app/**/*.html").then(function(resources) { @@ -48,7 +48,7 @@ test("FS RL: GLOB resources from application.a w/ virtual base path prefix", (t) })); }); -test("FS RL: GLOB resources from application.a w/o virtual base path prefix", (t) => { +test("FS-Adapter: GLOB resources from application.a w/o virtual base path prefix", (t) => { let readerWriters = t.context.readerWriters; // Get resource from one readerWriter return t.notThrows(readerWriters.source.byGlob("/**/*.html").then(function(resources) { @@ -56,7 +56,7 @@ test("FS RL: GLOB resources from application.a w/o virtual base path prefix", (t })); }); -test("Virtual RL: GLOB resources from application.a w/ virtual base path prefix", (t) => { +test("Mem-Adapter: GLOB resources from application.a w/ virtual base path prefix", (t) => { let readerWriter = ui5Fs.resourceFactory.createAdapter({ virBasePath: "/app/" }); @@ -75,7 +75,7 @@ test("Virtual RL: GLOB resources from application.a w/ virtual base path prefix" ); }); -test("Virtual RL: GLOB resources from application.a w/o virtual base path prefix", (t) => { +test("Mem-Adapter: GLOB resources from application.a w/o virtual base path prefix", (t) => { let readerWriter = ui5Fs.resourceFactory.createAdapter({ virBasePath: "/app/" }); @@ -94,8 +94,8 @@ test("Virtual RL: GLOB resources from application.a w/o virtual base path prefix ); }); -test("Virtual RL: GLOB virtual directory w/o virtual base path prefix", (t) => { - // TODO: Add similar test (globbing on empty directory) for FS RL +test("Mem-Adapter: GLOB virtual directory w/o virtual base path prefix", (t) => { + // TODO: Add similar test (globbing on empty directory) for FS-Adapter // TODO: Also add tests for nodir: true option let readerWriter = ui5Fs.resourceFactory.createAdapter({ virBasePath: "/app/"