Simplify the error handling slightly in the src/display/node_stream.js file

The various classes have `this._errored` and `this._reason` properties, where the first one is a boolean indicating if an error was encountered and the second one contains the actual `Error` (or `null` initially).

In practice this means that errors are basically tracked *twice*, rather than just once. This kind of double-bookkeeping is generally a bad idea, since it's quite easy for the properties to (accidentally) get into an inconsistent state whenever the relevant code is modified.

Rather than using a separate boolean, we can just as well check the "error" property directly (since `null` is falsy).

---

Somewhat unrelated to this patch, but `src/display/node_stream.js` is currently *not* handling errors in a consistent or even correct way; compared with `src/display/network.js` and `src/display/fetch_stream.js`.
Obviously using the `createResponseStatusError` utility function, from `src/display/network_utils.js`, might not make much sense in a Node.js environment. However at the *very* least it seems that `MissingPDFException`, or `UnknownErrorException` when one cannot tell that the PDF file is "missing", should be manually thrown.

As is, the API (i.e. `getDocument`) is not returning the *expected* errors when loading fails in Node.js environments (as evident from the `pending` API unit-test).
This commit is contained in:
Jonas Jenwald 2018-06-05 20:30:26 +02:00
parent 2fdaa3d54c
commit 547f119be6
2 changed files with 18 additions and 23 deletions

View File

@ -90,8 +90,7 @@ class BaseFullReader {
constructor(stream) {
this._url = stream.url;
this._done = false;
this._errored = false;
this._reason = null;
this._storedError = null;
this.onProgress = null;
let source = stream.source;
this._contentLength = source.length; // optional
@ -137,8 +136,8 @@ class BaseFullReader {
if (this._done) {
return Promise.resolve({ value: undefined, done: true, });
}
if (this._errored) {
return Promise.reject(this._reason);
if (this._storedError) {
return Promise.reject(this._storedError);
}
let chunk = this._readableStream.read();
@ -170,8 +169,7 @@ class BaseFullReader {
}
_error(reason) {
this._errored = true;
this._reason = reason;
this._storedError = reason;
this._readCapability.resolve();
}
@ -199,8 +197,8 @@ class BaseFullReader {
}
// Destroy ReadableStream if already in errored state.
if (this._errored) {
this._readableStream.destroy(this._reason);
if (this._storedError) {
this._readableStream.destroy(this._storedError);
}
}
}
@ -209,8 +207,7 @@ class BaseRangeReader {
constructor(stream) {
this._url = stream.url;
this._done = false;
this._errored = false;
this._reason = null;
this._storedError = null;
this.onProgress = null;
this._loaded = 0;
this._readableStream = null;
@ -228,8 +225,8 @@ class BaseRangeReader {
if (this._done) {
return Promise.resolve({ value: undefined, done: true, });
}
if (this._errored) {
return Promise.reject(this._reason);
if (this._storedError) {
return Promise.reject(this._storedError);
}
let chunk = this._readableStream.read();
@ -258,8 +255,7 @@ class BaseRangeReader {
}
_error(reason) {
this._errored = true;
this._reason = reason;
this._storedError = reason;
this._readCapability.resolve();
}
@ -281,8 +277,8 @@ class BaseRangeReader {
});
// Destroy readableStream if already in errored state.
if (this._errored) {
this._readableStream.destroy(this._reason);
if (this._storedError) {
this._readableStream.destroy(this._storedError);
}
}
}
@ -339,8 +335,7 @@ class PDFNodeStreamFullReader extends BaseFullReader {
}
this._request.on('error', (reason) => {
this._errored = true;
this._reason = reason;
this._storedError = reason;
this._headersCapability.reject(reason);
});
// Note: `request.end(data)` is used to write `data` to request body
@ -378,8 +373,7 @@ class PDFNodeStreamRangeReader extends BaseRangeReader {
}
this._request.on('error', (reason) => {
this._errored = true;
this._reason = reason;
this._storedError = reason;
});
this._request.end();
}
@ -398,8 +392,7 @@ class PDFNodeStreamFsFullReader extends BaseFullReader {
fs.lstat(path, (error, stat) => {
if (error) {
this._errored = true;
this._reason = error;
this._storedError = error;
this._headersCapability.reject(error);
return;
}

View File

@ -164,7 +164,9 @@ describe('api', function() {
});
it('creates pdf doc from non-existent URL', function(done) {
if (isNodeJS()) {
pending('XMLHttpRequest is not supported in Node.js.');
pending('Fix `src/display/node_stream.js` to actually throw ' +
'a `MissingPDFException` in all cases where a PDF file ' +
'cannot be found, such that this test-case can be enabled.');
}
var loadingTask = getDocument(
buildGetDocumentParams('non-existent.pdf'));