Skip to content

Commit

Permalink
[FIX] Prevent FS write from draining Resources content
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
RandomByte committed Jul 17, 2018
1 parent c7e031e commit 3a7c6a7
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 16 deletions.
12 changes: 9 additions & 3 deletions lib/AbstractReaderWriter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<undefined>} Promise resolving once data has been written
*/
write(resource) {
return this._write(resource);
write(resource, options = {final: false}) {
return this._write(resource, options);
}

/**
Expand All @@ -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<undefined>} Promise resolving once data has been written
*/
_write(resource) {
_write(resource, options) {
throw new Error("Not implemented");
}
}
Expand Down
11 changes: 9 additions & 2 deletions lib/Resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class Resource {
// }
this._stream = null;
this._buffer = buffer;
this._contentDrained = false;
}

/**
Expand Down Expand Up @@ -144,6 +145,7 @@ class Resource {
// this._stream.destroy();
// }
this._stream = stream;
this._contentDrained = false;
}

/**
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
});
Expand Down
33 changes: 28 additions & 5 deletions lib/adapters/FileSystem.js
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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<undefined>} 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);
Expand All @@ -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);
Expand Down
12 changes: 6 additions & 6 deletions test/lib/resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,23 @@ 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) {
t.deepEqual(resources.length, 1, "Found exactly one resource");
}));
});

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) {
t.deepEqual(resources.length, 1, "Found exactly one resource");
}));
});

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/"
});
Expand All @@ -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/"
});
Expand All @@ -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/"
Expand Down

0 comments on commit 3a7c6a7

Please sign in to comment.