Adds node.js logic for networking tasks for PDF.js #8712

mukulmishra18 · 2017-07-27T23:06:10Z

This PR implements IPDFStream interface in node.js environment. fs.createReadStream is used to create ReadableStream, that can be used to stream PDF data incrementally.

yurydelendik · 2017-07-27T23:29:29Z

src/display/node_stream.js

+};
+
+function PDFNetworkStreamRangeReader(path, begin, end) {
+  this._rangeRequest = fs.createReadStream(path, { begin, end, });


change begin=>start fs.createReadStream(path, { start, end, });

Done with new commit.

Snuffleupagus · 2017-07-28T07:34:32Z

src/display/node_stream.js

+let fs = require('fs');
+let assert = require('assert');
+
+function createPromiseCapabilities() {


Please don't duplicate this functionality, just import it from src/shared/util.js instead.

Done. Thanks.

yurydelendik

few more changes:

fs.lstat can be used to get length -- it is async so you can use it as a part of headersReady
rename PDFNetworkStream to PDFNodeStream (and class syntax can be used here)
at "src/pdf.js", import setPDFNetworkStream and depending on isNodeJS, use it to set PDFNetworkStream or PDFNodeStream.

yurydelendik · 2017-07-28T12:47:57Z

src/display/node_stream.js

+  },
+
+  cancel(reason) {
+    this._fullRequest.cancel(reason);


this._rangeRequeste.destroy(reason);

yurydelendik · 2017-07-28T12:48:18Z

src/display/node_stream.js

+}
+PDFNetworkStreamRangeReader.prototype = {
+  get isStreamingSupported() {
+


return true;

yurydelendik · 2017-07-28T12:48:44Z

src/display/node_stream.js

+  },
+
+  cancel(reason) {
+    this._fullRequest.close(reason);


this._fullRequest.destroy(reason);

yurydelendik · 2017-07-28T12:48:59Z

src/display/node_stream.js

+  },
+
+  get isStreamingSupported() {
+


return true;

Done. Thanks.

yurydelendik · 2017-07-28T12:49:03Z

src/display/node_stream.js

+  },
+
+  get isRangeSupported() {
+


return true;

Done. Thanks.

yurydelendik · 2017-07-28T12:49:49Z

src/display/node_stream.js

+PDFNetworkStreamFullReader.prototype = {
+
+  get contentLength() {
+


when fs.Stats are received, we will know stream length

Snuffleupagus · 2017-07-31T21:02:33Z

src/display/network.js

-    if (this._validateRangeRequestCapabilities()) {
+    // Setting right content length.
+    this._contentLength =
+      parseInt(this.getResponseHeader('Content-Length'), 10);


Won't just assigning a value, without doing any validation, mean that if the Content-Length header isn't present then this._contentLength will be NaN which probably isn't very good (compare with the old code which uses a !isInt(...) check)?

The following might not matter, but asking anyway since I'm not sure:
With this patch, you're always setting this._contentLength whereas before that was only done when range requests were supported; does this matter at all?

In refactoring patches we want to preserve initial logic as much as possible, so I recommend to return multiple values/object from validateRangeRequestCapabilities, e.g. {allowRangeRequests: ..., suggestedLength: ... }

this._contentLength will be NaN which probably isn't very good (compare with the old code which uses a !isInt(...) check)?

Yeah, you are right. Sorry I didn't noticed that. I will add check for isInt. Thanks.

you're always setting this._contentLength whereas before that was only done when range requests were supported

This commit is not intended to change any functionality of the network.js file(just to extract the logic for range request support). I think setting this._contentLength inside if (validateRangeRequestCapability) {... would be better. Thanks for pointing out.

return multiple values/object from validateRangeRequestCapabilities, e.g. {allowRangeRequests: ..., suggestedLength: ... }

Okay, we'll do this way. Thanks.

Rob--W

I did a cursory review, and found some issues.

Some observations:

It seems that you have based the logic on src/display/transport_stream.js. This is not necessarily a bad thing, but I wonder whether this indeed makes most sense in Node.js.
The implementation does not try to minimize resource usage. At the very least, once a request has finished and the response has yielded, it is probably sufficient to discard the underlying handle to the data (e.g. the IncomingMessage).
There are no unit tests that demonstrates the new functionality, let alone that the functionality works as expected. This could have caught some of the bugs that I mentioned below. Please add them. If you need inspiration: Currently we have one Node.js-specific test, namely https://github.com/mozilla/pdf.js/blob/5b5781b45d234666241bf3354c0d390315c31d1a/test/unit/display_svg_spec.js.

Rob--W · 2017-08-03T22:11:30Z

src/display/node_stream.js

+    this.source = options.source;
+    this.url = url.parse(this.source.url);
+    this.isHttp = this.url.protocol === 'http' ||
+                  this.url.protocol === 'https';


the protocol property of a parsed URL ends with a colon, so replace 'http' with 'http:' (similarly for 'https').

Done. Thanks for pointing out.

Rob--W · 2017-08-03T22:23:39Z

src/display/node_stream.js

+      method: 'GET',
+      headers: stream.httpHeaders,
+    };
+    this.request = http.request(options, (response) => {


You have to check whether this._url.protocol is https:, and if so, use the https.request, and http.request otherwise. Similarly for other places where you use http.request.

Done. Thanks for the advice.

Rob--W · 2017-08-03T22:32:12Z

src/display/node_stream.js

+      method: 'GET',
+      headers: stream.httpHeaders,
+    };
+    this.request = http.request(options, (response) => {


this.request should be private, this._request at the very least.

And also see my previous comment about http.request.

Done. Thanks.

Rob--W · 2017-08-03T22:45:39Z

src/display/node_stream.js

+  }
+
+  cancel(reason) {
+    this._rangeRequest.cancel(reason);


_rangeRequest should not be a property of the base class, because the base class does not need to know about the implementation detail, so you should remove the use of _rangeRequest here. Looking at the implementation, you should probably not directly expose _rangeRequest, but possibly store a reference to the .read method, maybe as _read.
Besides, the _rangeRequest object is not the same in the subclasses:

The argument of the callback to http.request is an IncomingMessage, which does not have a cancel method - https://nodejs.org/api/http.html#http_class_http_incomingmessage

In PDFNodeStreamFsRangeReader, the type is a ReadableStream, which does not have a cancel method either.

http.request is an IncomingMessage, which does not have a cancel method

Sorry for typo. This should be this._rangeRequest.close(reason)

but possibly store a reference to the .read method, maybe as _read.

Sorry, but I really don't understand your concern here. Should we have to remove cancel from BaseRangeReader class, and also not call this._rangeReder.read() directly in read() method ?
Can you please explain it a bit?

but possibly store a reference to the .read method, maybe as _read.

As far as I understand, do you mean to store reference of .read method of this._rangeRequest in _read property of PDFNodeStreamRangeReader(same for PDFNodeStreamFsRangeReader), somthing like: this._read = this._rangeRequest.read, and then use this._read inside read() of base class?

Rob--W · 2017-08-03T22:46:03Z

src/display/node_stream.js

+    this._loaded = 0;
+    this._readCapability = createPromiseCapability();
+
+    this._rangeRequest = null;


This should be removed, see my note below about _rangeRequest.

Rob--W · 2017-08-03T22:50:51Z

src/display/network.js

@@ -593,8 +569,6 @@ PDFNetworkStreamRangeRequestReader.prototype = {
  },
 };

-setPDFNetworkStreamClass(PDFNetworkStream);


Why this change?

In src/pdf.js, we are trying to detect the environment(using isNodeJS), and then setting PDFNodeStream or PDFNetworkStream, see one of the TODO here: #8712 (review)

But in non-Node.js, the stream class is no longer set. That's not good.

It is safe to repeatedly call setPDFNetworkStreamClass, so restore this line.

webpack can package node_stream and network in one file and initialize these in some random order. so it's better to call setPDFNetworkStreamClass from api.js and remove this line from the network.js

yurydelendik · 2017-08-04T19:23:47Z

src/display/network.js

  },

  _onHeadersReceived:
      function PDFNetworkStreamFullRequestReader_onHeadersReceived() {
+    let { allowRangeRequests, suggestedLength, } =
+      validateRangeRequestCapabilities({
+        getResponseHeader: this.getResponseHeader.bind(this),


don't bind() -- create a function here e.g. getResponseHeader: (name) => { return fullRequestXhr.getResponseHeader(name); },

Done. Thanks.

yurydelendik · 2017-08-04T19:26:57Z

src/display/node_stream.js

+
+  cancel(reason) {
+    this._fullRequest.close(reason);
+    this._fullRequest.destroy(reason);


not sure if we need two of these. can we use either close() or destroy()?

Removed this._fullRequest.close(reason);. Thanks.

yurydelendik · 2017-08-04T19:27:42Z

src/display/node_stream.js

+  }
+
+  get isStreamingSupported() {
+    return false;


I prefer to have it set to true here, if it's enabled in options.

yurydelendik · 2017-08-04T19:29:29Z

src/display/node_stream.js

+    }
+
+    this._isStreamingSupported = !stream.source.disableStream;
+    this._isRangeSupported = false;


I prefer to have the above options applied for Fs readers, can we move them into base reader?

yurydelendik · 2017-08-04T19:31:13Z

src/display/node_stream.js

+        this._isRangeSupported = true;
+      }
+      this._length = suggestedLength;
+    });


Move this entire function body into the handleResponse -- notice that this._headersCapability.resolve() still has to be called.

Done. Thanks.

Rob--W

This looks better than before. I am still expecting some unit tests to ascertain that the introduced logic works as expected (see my previous review for more details).

Rob--W · 2017-08-06T22:30:38Z

src/display/network.js

@@ -593,8 +569,6 @@ PDFNetworkStreamRangeRequestReader.prototype = {
  },
 };

-setPDFNetworkStreamClass(PDFNetworkStream);


But in non-Node.js, the stream class is no longer set. That's not good.

It is safe to repeatedly call setPDFNetworkStreamClass, so restore this line.

Rob--W · 2017-08-06T22:37:49Z

src/display/node_stream.js

+    this._length = stream.source.length;
+    this._loaded = 0;
+
+    this._fullRequest = null;


This name is a bit confusing, because you use the same name elsewhere in the file.
Furthermore, _fullRequest is not an accurate description of the value.

How about renaming _fullRequest of the *FullReader classes to _readableStream? This would also make the code more understandable for those who are already familiar with Node.js streams.

How about renaming _fullRequest of the *FullReader classes to _readableStream?

Sounds like a good idea. Do you think we should also rename _rangeRequest with _readableStream(?) in *RangeReader?

Rob--W · 2017-08-06T22:45:03Z

src/display/node_stream.js

+    this.url = url.parse(this.source.url);
+    this.isHttp = this.url.protocol === 'http:' ||
+                  this.url.protocol === 'https:';
+    this.isFsUrl = !this.url.host;


Is isFsUrl supposed to say whether a URL refers to the filesystem?
If so, this is not the right way to say so. The file: scheme is commonly used to refer to local files, and it can have a host part too - see e.g. https://en.wikipedia.org/wiki/File_URI_scheme

What are the expected values for url that you intend to support here?

What are the expected values for url that you intend to support here?

I am checking here if url refers to the filesystem or not, so that we can choose between PDFNodeStream***Reader or PDFNodeStreamFs***Reader.

I am trying to parse URL using url.parse, and then checking for host. In most of cases I found that urls are of the form ../....path/to/some/pdf.pdf or if url is absolute home/userName/path/to/some/pdf.pdf or file:///home/user/path/to/some/pdf.pdf-- (and all these cases works fine with above check). The only case that I found failing is file://localhost/path/to/some/pdf.pdf, and I think this can be fix by checking for protocol === file:.

Is it sounds reasonable to you, or you have something else in your mind? If not, can you suggest me something concrete?

The canonical URL of a local file is a file:-URL, so I suggest to check whether the scheme is file:.

mukulmishra18 · 2017-08-08T11:56:46Z

I am trying to add some tests for node_stream logic[1]. While writing test I find out that following #8712 (comment), breaks rangeReader.read() operation, so we are not getting anything while reading here. To fix this we have to shift read() in **RangeReaders class from BaseRangeReader class, and call this._rangeReader.read() instead.

Although running node_stream_spec separately I am getting all output as expected, but I am facing some problems, mainly here.

When running gulp unittest, I am getting __non_webpack_require__ is not defined.
When running gulp unittestcli, I am getting different value for pdf2 as excepted(Getting: home/user/pdf.js/build/lib/test/pdfs/tracemonkey.pdf, expected: home/user/pdf.js/test/pdfs/tracemonkey.pdf).

[1]: Note that I am following network_spec file to write tests, as node_stream is just adding IPDFStream for node.js environment.

yurydelendik · 2017-08-14T17:02:03Z

src/display/network.js

@@ -14,10 +14,11 @@
 */

 import {
-  assert, createPromiseCapability, globalScope, isInt, MissingPDFException,
+  assert, createPromiseCapability, globalScope, MissingPDFException,
  UnexpectedResponseException
 } from '../shared/util';
 import { setPDFNetworkStreamClass } from './api';


remove setPDFNetworkStreamClass import from this file

I think there is some confusion at #8712 (comment). Done with new commit, Thanks.

Rob--W · 2017-08-15T09:23:28Z

src/display/node_stream.js

+        return Promise.reject(this._reason);
+      }
+
+      let chunk = this._rangeRequest.read();


This is duplication of code. The two implementations of read are identical.

What stops you from putting read in the base class?
If -for some reason- you cannot use this._rangeRequest.read() in the base class, then you can create a method in the subclass (e.g. _read() { return this._rangeRequest.read(); } and call this._read(); from the base class instead.

And also, I thought that you had replaced "_rangeRequest" with "_readableStream", but I can not find that change (any more?).

What stops you from putting read in the base class?

Sorry, but not sure why when I am doing something like #8712 (comment), I am unable to read from the stream, see #8712 (comment).

Also _read() { return this._rangeReques.read(); } creates the same issue.

And also, I thought that you had replaced "_rangeRequest" with "_readableStream", but I can not find that change (any more?).

Done with new commit. Thanks.

The context is missing from the comment that you've linked. I think that the breakage is unrelated to the method being in the base class. You now literally have two (actually, after looking again THREE, BaseFullReader.prototype.read is almost identical, except with the "length" property in onProgress) identical methods read in the sub classes.

The inheritance tree looks like this:

BaseRangeReader

PDFNodeStreamRangeReader extends BaseRangeReader

PDFNodeStreamFsRangeReader extends BaseRangeReader

Your current code duplicates the read method in the latter two classes (the implementation is byte for byte identical). The runtime behavior is identical regardless of where you put the implementation of the "read" method, so you should put the implementation in the base class (BaseRangeReader), to improve the maintainability of the code.

Rob--W · 2017-08-15T09:36:06Z

Although running node_stream_spec separately I am getting all output as expected, but I am facing some problems, mainly here.

When running gulp unittest, I am getting __non_webpack_require__ is not defined.

It seems that we are not passing the right babelOptions to the transpiler when running tests in the browser.

But even if we do, then the error should still be "require is not defined" or something like "require("fs") is not a module", because it does not make any sense to run the Node.js-only test in the browser.

At minimum, skip the test when you detect that the environment is not Node, e.g. as done here:

pdf.js/test/unit/display_svg_spec.js

Lines 112 to 114 in ba5dbc9

    
           if (!isNodeJS()) { 
        
             pending('zlib.deflateSync is not supported in non-Node environments.'); 
        
           }

Rob--W · 2017-08-15T10:23:14Z

I've submitted a patch for the __non_webpack_require__ issue at #8781. Note that an error is still expected, the true solution is to not load the module in non-Node.js environments.

mukulmishra18 · 2017-08-16T08:27:26Z

At minimum, skip the test when you detect that the environment is not Node.

Okay, sure. Tests for node_stream is now removed from gulp unittest and only covered in gulp unittestcli.

yurydelendik · 2017-08-17T15:36:05Z

src/display/node_stream.js

+
+    this._readableStream = null;
+    let rangeStr = start + '-' + (end - 1);
+    stream.httpHeaders['Range'] = 'bytes=' + rangeStr;


create new object for httpHeaders (before 'Range' is changed) -- we don't want to pollute original httpHeaders object.

Done in new commit. Thanks,

yurydelendik · 2017-08-17T15:36:56Z

src/display/node_stream.js

+
+    this._readableStream = null;
+    let rangeStr = start + '-' + (end - 1);
+    stream.httpHeaders['Range'] = 'bytes=' + rangeStr;


use bytes=${start}-${end - 1} format

yurydelendik · 2017-08-17T15:38:14Z

src/display/node_stream.js

+      path: this._url.path,
+      method: 'GET',
+      headers: stream.httpHeaders,
+    };


Move options creating into separate function (we have similar code above) and add auth and port field, also probably agent.

Created createRequestOptions function that returns options object. Also tried to add a simple http.Agent, please check if it looks good to you. Thanks.

yurydelendik · 2017-08-17T15:45:54Z

src/display/node_stream.js

+        this._errored = true;
+        this._reason = reason;
+        this._readCapability.resolve();
+      });


move all .on('readable'/'end'/'error', into e.g. private _setReadableStream into base class -- it can be use by Fs constructor as well.

The same thing needs to be done for "full" readers too.

Done in new commit.

Note: I have moved this._readableStream property in BaseRangeReader from concrete classes and it works just fine for me.

yurydelendik · 2017-08-17T15:46:46Z

src/display/node_stream.js

+      }
+      return Promise.resolve({ value: chunk, done: false, });
+    });
+  }


agree with moving read() and cancal() into base class

Done in new commit. Thanks.

yurydelendik · 2017-08-17T15:48:48Z

src/display/node_stream.js

+  constructor(stream, start, end) {
+    super(stream);
+
+    this._readableStream = fs.createReadStream(this._url.path, { start, end, });


as mentioned above, use this._setReadableStream(fs.createReadStream(this._url.path));

yurydelendik · 2017-08-17T15:50:52Z

src/display/network_utils.js

+import { isInt } from '../shared/util';
+
+function validateRangeRequestCapabilities({ getResponseHeader, isHttp,
+                                            rangeChunkSize, disableRange, }) {


can you add assert(rangeChunkSize > 0); here

Done. Thanks.

yurydelendik · 2017-08-17T15:51:53Z

src/display/node_stream.js

+    this._rangeChunkSize = stream.source.rangeChunkSize;
+    if (!this._rangeChunkSize && !this._disableRange) {
+      this._disableRange = true;
+    }


move this_disableRange and this._rangeChunkSize logic into base class.

Done. Thanks.

yurydelendik · 2017-08-18T18:19:17Z

src/display/node_stream.js

+
+      let { allowRangeRequests, suggestedLength, } =
+      validateRangeRequestCapabilities({
+        getResponseHeader: this.getResponseHeader.bind(this),


remove getResponseHeader function below and inline the response.headers access here

Done. Thanks.

yurydelendik · 2017-08-18T18:19:49Z

src/display/node_stream.js

+    keepAlive: true,
+    maxSockets: 10,
+  };
+  let agent = url.protocol === 'http:' ? new http.Agent(agentOptions) :


too complicated, let's remove agent for now.

yurydelendik · 2017-08-18T18:27:33Z

src/display/node_stream.js

+  }
+
+  cancel(reason) {
+    this._readableStream.destroy(reason);


here and at the range base reader, we need to think "what if cancel is called before _readableStream is set". I recommend to extract on('error' body into _error(reason) method and call it when this._readableStream is not set. in _setReadableStream we can check if this._errored = true we can quickly destroy it.

Done in new commit. Thanks.

yurydelendik · 2017-08-21T20:04:03Z

I see UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Bad end offset: 65537 when running getinfo.js example (with getDocument(pdfPath) change).

yurydelendik · 2017-08-21T20:06:11Z

If I pass HTTP URL is the getinfo.js I get the following error:

Error: Cannot find module '/Users/yury/Work/pdf.js/examples/node/http:/mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf'

mukulmishra18 · 2017-08-21T21:35:09Z

Error: Cannot find module '/Users/yury/Work/pdf.js/examples/node/http:/mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf'

Strange! seems like we are getting wrong url in node_stream.js. Although on running getinfo.js locally, I find out that I am getting some initial chunk of data, but not all. I am doing something like:

var pdfjsLib = require('../../build/lib/pdf.js');

pdfjs.getDocument('http://mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf').
  then((doc) => {...});

Although, earlier running the above version of getinfo.js gives me correct output, seems like using read() and this._readableStream in BaseRangeReader causing this error.

mukulmishra18 · 2017-08-23T09:04:50Z

Okay, I find out the problem. The headers received by http.requestin node.js environment is in lower case(also mentioned here: https://nodejs.org/api/http.html#http_message_headers), but not in browser(when requesting using XHR or fetch) Example: Accept-Ranges in XHR or fetch whereas accept-ranges in node.js.

This create problem here: mukulmishra18@3493f14#diff-35bcb37ef640f9436dd67f5077c08b55R271 -- (returns undefined when trying to access Accept-Ranges). Changing the requested header to lower case solves the problem.

getinfo.js example is now working fine for me on my local system. @yurydelendik can you please check, if it works for you?

Rob--W · 2017-08-23T09:36:05Z

test/unit/node_stream_spec.js

+
+describe('node_stream', function() {
+  let pdf1 =
+    'http://mozilla.github.io/pdf.js/web/compressed.tracemonkey-pldi-09.pdf';


This results in an external network request, and slow down the unit test if network is slow, or even cause the unit test to fail if network is somehow unavailable.

Please use a local file server in Node for testing. Don't forget to close the server regardless of test failure. To do so you can use http.createServer in a beforeAllhook (and start the server on a random free port using.listen(0), and call .close()on the server in anafterAllhook. The return value of.listenhas anaddressmethod that contains the random port in theportproperty. After starting the server, you can use something likehttp://127.0.0.1:${port}/tracemonkey.pdf`.

And for the browser... Why does this test have a special case !isNode() if the API that you are testing is only supposed to work on Node? Am I missing something?

this script is not added to jasmine-bootstrap.js -- let's also add assert(isNode()); just in case somebody will decide to add it.

Why does this test have a special case !isNode() if the API that you are testing is only supposed to work on Node?

Sorry, removed with new commit, as we don't that any more.

Please use a local file server in Node for testing

Added with new commit. Can you please check if it looks good to you?

let's also add assert(isNode()); just in case somebody will decide to add it.

Done. Thanks.

Rob--W · 2017-08-23T09:39:40Z

test/unit/node_stream_spec.js

+    let path = __non_webpack_require__('path');
+    let url = __non_webpack_require__('url');
+    pdf2 =
+      url.parse(path.join(process.cwd(), './test/pdfs/tracemonkey.pdf')).href;


In this case, pdf2 looks like /path/to/pdf.js/test/pdfs/tracemonkey.pdf.
I was expecting a file:// prefix, because otherwise it would not be a URL. Does the test really pass? Regardless, can you make sure that the url really looks like a URL and not a path?

I was expecting a file:// prefix, because otherwise it would not be a URL.

Then I think adding file:// with process.cwd() will help, something like : url.parse(path.join('file://' + process.cwd(), './test/pdfs/tracemonkey.pdf'))

yurydelendik · 2017-08-23T14:35:35Z

src/display/node_stream.js

+    super(stream);
+
+    this._setReadableStream(
+      fs.createReadStream(this._url.path, { start, end, }));


I still have (node:90529) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Bad end offset: 65537. Node.js operates with end being inclusive. Make it end: end - 1,

Done in new commit. Thanks.

yurydelendik · 2017-08-23T14:36:59Z

test/unit/node_stream_spec.js

+    let range12Reader = stream1.getRangeReader(pdfLength - tailSize, pdfLength);
+
+    let range21Reader = stream2.getRangeReader(pdfLength - tailSize - rangeSize,
+                                             pdfLength - tailSize - 1);


Must be pdfLength - tailSize without - 1 based on our API definition.

Done. Thanks.

yurydelendik

looks good, with Rob's requested changes and off-by-one fix in the fs range reader

yurydelendik · 2017-08-23T14:51:21Z

let's remove fs.readFileSync() from the getinfo.js example to show that we implemented this functionality and it works

Snuffleupagus · 2017-08-23T21:08:20Z

test/unit/node_stream_spec.js

+import { assert, isNodeJS } from '../../src/shared/util';
+import { PDFNodeStream } from '../../src/display/node_stream';
+
+// Make sure that we are not running this script is Node.js environment.


Nit: Hasn't this comment accidentally been inverted, since I'm assuming that it should be e.g. Make sure that we are only running this script in Node.js environments.?

Oops..., Thanks for pointing out.

yurydelendik · 2017-08-24T14:37:55Z

/botio test

pdfjsbot · 2017-08-24T14:37:56Z

From: Bot.io (Windows)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://54.215.176.217:8877/c7f9509b9d69410/output.txt

pdfjsbot · 2017-08-24T14:37:56Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @yurydelendik received. Current queue size: 0

Live output at: http://54.67.70.0:8877/2ee68f404042106/output.txt

pdfjsbot · 2017-08-24T14:54:26Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/2ee68f404042106/output.txt

Total script time: 16.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-08-24T15:07:40Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/c7f9509b9d69410/output.txt

Total script time: 29.72 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

yurydelendik · 2017-08-24T16:35:42Z

Thank you for the patch.

Adds node.js logic for networking tasks for PDF.js

yurydelendik added the 4-work-in-progress label Jul 27, 2017

yurydelendik reviewed Jul 27, 2017

View reviewed changes

Snuffleupagus reviewed Jul 28, 2017

View reviewed changes

yurydelendik reviewed Jul 28, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch 2 times, most recently from 67aaf16 to 712de53 Compare July 28, 2017 20:04

timvandermeij added the core label Jul 28, 2017

Snuffleupagus reviewed Jul 31, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch 2 times, most recently from c30ca67 to de29337 Compare August 3, 2017 19:49

Rob--W requested changes Aug 3, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from de29337 to b148a29 Compare August 4, 2017 19:07

yurydelendik reviewed Aug 4, 2017

View reviewed changes

Rob--W requested changes Aug 6, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from 6e9f924 to f7aa67b Compare August 10, 2017 07:36

yurydelendik reviewed Aug 14, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch 2 times, most recently from c4bad64 to d3537c0 Compare August 14, 2017 19:06

Rob--W reviewed Aug 15, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from d3537c0 to d6e330c Compare August 16, 2017 08:23

yurydelendik reviewed Aug 17, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from d6e330c to f0b7682 Compare August 17, 2017 22:33

yurydelendik reviewed Aug 18, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from f0b7682 to 0ce321b Compare August 18, 2017 21:26

mukulmishra18 force-pushed the node_stream branch from 0ce321b to c9a9806 Compare August 23, 2017 08:16

mukulmishra18 added 2 commits August 23, 2017 14:06

Adds node.js logic for networking tasks for PDF.js

ed78b23

Adds http support to node_stream logic

18ede8c

mukulmishra18 force-pushed the node_stream branch 2 times, most recently from 2250094 to 3493f14 Compare August 23, 2017 08:47

Rob--W requested changes Aug 23, 2017

View reviewed changes

yurydelendik reviewed Aug 23, 2017

View reviewed changes

yurydelendik approved these changes Aug 23, 2017

View reviewed changes

timvandermeij removed the 4-work-in-progress label Aug 23, 2017

mukulmishra18 force-pushed the node_stream branch from 3493f14 to 8414ac6 Compare August 23, 2017 21:02

Snuffleupagus reviewed Aug 23, 2017

View reviewed changes

mukulmishra18 force-pushed the node_stream branch from 8414ac6 to 4bf45cc Compare August 23, 2017 21:54

Adds tests for node_stream

d16709f

mukulmishra18 force-pushed the node_stream branch from 4bf45cc to d16709f Compare August 24, 2017 07:17

yurydelendik merged commit e82811a into mozilla:master Aug 24, 2017

Snuffleupagus mentioned this pull request Sep 5, 2017

Fix the helloworld example by setting the PDFNetworkStream class (PR 8712 follow-up) #8874

Merged

njakob mentioned this pull request Sep 14, 2017

ENOENT: no such file or directory, lstat #8907

Closed

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8712 from mukulmishra18/node_stream

e6c2399

Adds node.js logic for networking tasks for PDF.js

		PDFNetworkStreamFullReader.prototype = {

		get contentLength() {

Adds node.js logic for networking tasks for PDF.js #8712

Adds node.js logic for networking tasks for PDF.js #8712

Conversation

mukulmishra18 commented Jul 27, 2017

yurydelendik Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukulmishra18 Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukulmishra18 Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukulmishra18 commented Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukulmishra18 Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented Aug 15, 2017

Rob--W commented Aug 15, 2017

mukulmishra18 commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jul 27, 2017 •

edited

Loading

yurydelendik Jul 28, 2017 •

edited

Loading

mukulmishra18 Jul 31, 2017 •

edited

Loading

mukulmishra18 Aug 4, 2017 •

edited

Loading

mukulmishra18 commented Aug 8, 2017 •

edited

Loading

mukulmishra18 Aug 16, 2017 •

edited

Loading