From 547f119be628f58e12237addd528ea5239b80e6c Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Tue, 5 Jun 2018 20:30:26 +0200
Subject: [PATCH] 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).
---
 src/display/node_stream.js | 37 +++++++++++++++----------------------
 test/unit/api_spec.js      |  4 +++-
 2 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/src/display/node_stream.js b/src/display/node_stream.js
index 3332d46d9..84ab37a8b 100644
--- a/src/display/node_stream.js
+++ b/src/display/node_stream.js
@@ -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;
       }
diff --git a/test/unit/api_spec.js b/test/unit/api_spec.js
index 3b0cdf23f..1d3264649 100644
--- a/test/unit/api_spec.js
+++ b/test/unit/api_spec.js
@@ -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'));