[fix]: Removed synchronous operation with file system. #879

CheerlessCloud · 2017-09-26T04:49:21Z

I'm completely discouraged by this code. @aslushnikov How could you write such a thing?
I would somehow understand this solution if the code were written on callbacks. But you use the async
function! And inside the asynchronous function you are happy to block the EventLoop. What for?

If someone tells me where to put this "promisify", I'll move it without problems. But still I recommended somehow to solve this problem in the near future.

The remaining cases use synchronous writing and reading are not as critical, but it is advisable to fix them too: 1, 2.

googlebot · 2017-09-26T04:49:23Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

CheerlessCloud · 2017-09-26T05:17:50Z

I signed it!

googlebot · 2017-09-26T05:17:52Z

CLAs look good, thanks!

Garbee

LGTM

ebidel · 2017-09-26T15:22:12Z

lib/Page.js

@@ -26,6 +26,12 @@ const {Keyboard, Mouse, Touchscreen} = require('./Input');
 const Tracing = require('./Tracing');
 const {helper, debugError} = require('./helper');

+function writeFileAsync(...options) {


@aslushnikov , should this be added in helper? I could use it in #881.

As it seems to me, in a certain file/folder "utils" or "helpers" it is necessary to make the function promisify (as here) and use it to create wrappers of the fs methods or some other in local module scope (as here). Or we can add some external dependency, such as Bluebird or pify.
IMHO, I, as a person external to the project, can not make such decisions. And I do not want to rewrite the code, so let's work out solution and I will implement it in this PR. This is almost my first PR to open source project, so I may not know some rules, sorry =)

@CheerlessCloud let's add both promisify and writeFileAsync into helper.

aslushnikov · 2017-09-27T02:17:12Z

Hey @CheerlessCloud, thank you for the pull request!

I'm completely discouraged by this code. @aslushnikov How could you write such a thing?
I would somehow understand this solution if the code were written on callbacks. But you use the async
function! And inside the asynchronous function you are happy to block the EventLoop. What for?

writeFileSync is bad. Thanks to your passion, the software is getting better!

The remaining cases use synchronous writing and reading are not as critical, but it is advisable to fix them too: 1, 2.

I'd be happy to review PRs.

…ion by method from helper.

CheerlessCloud · 2017-09-27T07:04:01Z

Do not you think that the file "helper" for 300+ lines is antipattern? What about separate his to files, single file for one method? Yes, this does not completely solve the problem, but it will make it a little easier.

aslushnikov · 2017-09-27T09:19:33Z

Do not you think that the file "helper" for 300+ lines is antipattern?

I think its fine.

aslushnikov · 2017-09-27T09:22:18Z

lib/Launcher.js

@@ -122,7 +127,7 @@ class Launcher {
          process.kill(-chromeProcess.pid, 'SIGKILL');
        // Attempt to remove temporary profile directory to avoid littering.
        try {
-          removeSync(temporaryUserDataDir);


This should stay sync: handlers for process exit are expected to complete synchronously.

aslushnikov · 2017-09-27T09:23:27Z

lib/Tracing.js

 const {helper} = require('./helper');
+const fs = require('fs');
+
+fs.openAsync = helper.promisify(fs.open);


let's not spoil the globals

const openAsync = helper.promisify(fs.open); const writeAsync = helper.promisify(fs.write); const closeAsync = helper.promisify(fs.close);

aslushnikov · 2017-09-27T09:27:05Z

lib/Launcher.js

@@ -15,16 +15,18 @@
 */
 const os = require('os');
 const path = require('path');
-const removeSync = require('rimraf').sync;
+const {helper} = require('./helper');
+const rimrafAsync = helper.promisify(require('rimraf'));


nit: let's name this removeAsync: the rimraf doesn't say much of what the function is doing.

aslushnikov · 2017-09-27T09:27:50Z

lib/Launcher.js

 const ChromiumRevision = require('../package.json').puppeteer.chromium_revision;

+fs.mkdtempAsync = helper.promisify(fs.mkdtemp);


let's not spoil the global:

const mkdtempAsync = helper.promisify(fs.mkdtemp);

aslushnikov · 2017-09-27T09:33:32Z

lib/helper.js

+   *  const fs = Helper.promisifyAll(require('fs'));
+   *  fs.writeFileAsync(...); // -> Promise
+   */
+  static promisifyAll(object) {


we don't need this method

Well, I can remove it, but it may be needed in the future.

Thanks, we'll add it if/when the need arises.

aslushnikov · 2017-09-27T09:37:28Z

lib/helper.js

@@ -225,6 +226,86 @@ class Helper {
  static isNumber(obj) {
    return typeof obj === 'number' || obj instanceof Number;
  }
+
+  /**


let's not fallback to util - the helper.promisify implementation is simple enough for our use case. It's easier to read and maintain a single code path.

/** * @param {function(?)} nodeFunction * @return {function(?):!Promise<?>} */ static promisify(nodeFunction) { /** * @param {!Array<?>} options * @return {!Promise<?>} */ return function(...options) { return new Promise(function(fulfill, reject) { options.push(callback); nodeFunction.call(null, ...options); function callback(err, result) { if (err) reject(err); else fulfill(result); } }); }; }

By "not fallback to util" you mean using util.promisify.custom symbol?

yes, and also the util.promisify method.

I'd just stick with the simple shim we already have somewhere.

you can use https://npmjs.com/util.promisify if you want a compatible shim.

aslushnikov · 2017-09-27T09:38:16Z

lib/helper.js

@@ -225,6 +226,86 @@ class Helper {
  static isNumber(obj) {
    return typeof obj === 'number' || obj instanceof Number;
  }
+
+  /**
+   * @static


we don't use @static, @throws and @example declarations. They are harmless, but I'd prefer them to be removed for consistency with the rest of the code.

aslushnikov · 2017-09-27T09:44:55Z

lib/Launcher.js

@@ -90,8 +92,11 @@ class Launcher {
      chromeProcess.once('close', () => {
        // Cleanup as processes exit.
        if (temporaryUserDataDir)
-          removeSync(temporaryUserDataDir);
-        fulfill();
+          rimrafAsync(temporaryUserDataDir)


nit: braces are required around multi-line branches:

if (temporaryUserDataDir) { rimrafAsync(..) .. .... } else { fulfill(); }

aslushnikov · 2017-09-27T09:47:19Z

lib/Launcher.js

-        fulfill();
+          rimrafAsync(temporaryUserDataDir)
+            .then(() => fulfill())
+            .catch(err => console.error(err));


let's reject the waitForChromeToClose promise

In a few cases we don't catch errors from this promise, so we can get an unhandledRejection.

…m and delete promisifyAll method.

Rename rimrafAsync to removeFolderAsync. Place mkdtempAsync locally. Use removeFolder.sync in killChrome function.

petrgrishin · 2017-09-28T05:41:54Z

👍

graingert · 2017-09-28T18:37:53Z

lib/Launcher.js

@@ -86,12 +89,16 @@ class Launcher {
      chromeProcess.stderr.pipe(process.stderr);
    }

-    const waitForChromeToClose = new Promise(fulfill => {
+    const waitForChromeToClose = new Promise((fulfill, reject) => {


Don't mix new Promise, x.then and async/await. Convert callback apis into promises ASAP, then switch back to the highest abstraction.

const waitForCloseEvent = () => new Promise(resolve => chromeProcess.once('close', resolve); const waitForChromeToClose = (async () => { await waitForCloseEvent(); if (temporaryUserDateDir) { try { await removeFolderAsync(temporaryUserDataDir); } catch(e) { console.error(e); // are you sure, what's wrong with letting it throw? } } })();

@graingert I don't see much benefits in the approach you suggested, nor it is easier for me to read. I'd say the current code is just good enough.

@aslushnikov I disagree, new Promise should not be mixed with x.then.

@graingert I might be misunderstanding something. Lets split this discussion out of this thread: would you mind sending a separate PR that does changes that you find important and explains why are they necessary?

graingert · 2017-09-28T18:39:32Z

lib/Page.js

@@ -585,7 +587,7 @@ class Page extends EventEmitter {

    const buffer = new Buffer(result.data, 'base64');
    if (options.path)
-      fs.writeFileSync(options.path, buffer);
+      await writeFileAsync(options.path, buffer);


Why is there even an option to write the file here? Either puppeteer should return the actual stream or users should just write the buffer themselves.

@graingert two reasons:

it comes handy from user standpoint: the majority of page.screenshot usages have path option in it.

it's cheap from the project standpoint.

graingert · 2017-09-28T18:40:47Z

lib/helper.js

+   * @param {function(?)} nodeFunction
+   * @return {function(?):!Promise<?>}
+   */
+  static promisify(nodeFunction) {


why not use @ljharb's wonderful https://www.npmjs.com/package/util.promisify

@graingert no need to pull in a dependency for 30 lines of code.

when it's reliable and faster in modern node, it's always better to have more deps :-)

graingert · 2017-09-28T18:43:02Z

lib/Launcher.js

+        if (temporaryUserDataDir) {
+          removeFolderAsync(temporaryUserDataDir)
+              .then(() => fulfill())
+              .catch(err => console.error(err));


why not just let this throw, like removeSync would have done.

@graingert to avoid unhandledPromiseRejection.

presumably people are going to await launch(); and thus catch any errors

How's Page.pdf related to this code?

@aslushnikov ah you're right. Edited comment

aslushnikov

Thank you @CheerlessCloud for working on this. I left a few minor comments before we can merge this.

aslushnikov · 2017-09-29T18:55:44Z

lib/Launcher.js

@@ -86,12 +89,16 @@ class Launcher {
      chromeProcess.stderr.pipe(process.stderr);
    }

-    const waitForChromeToClose = new Promise(fulfill => {
+    const waitForChromeToClose = new Promise((fulfill, reject) => {


nit: reject is not used

aslushnikov · 2017-09-29T18:58:25Z

lib/Launcher.js

@@ -86,12 +89,16 @@ class Launcher {
      chromeProcess.stderr.pipe(process.stderr);
    }

-    const waitForChromeToClose = new Promise(fulfill => {
+    const waitForChromeToClose = new Promise((fulfill, reject) => {


@graingert I don't see much benefits in the approach you suggested, nor it is easier for me to read. I'd say the current code is just good enough.

aslushnikov · 2017-09-29T19:00:40Z

lib/Page.js

@@ -585,7 +587,7 @@ class Page extends EventEmitter {

    const buffer = new Buffer(result.data, 'base64');
    if (options.path)
-      fs.writeFileSync(options.path, buffer);
+      await writeFileAsync(options.path, buffer);


@graingert two reasons:

it comes handy from user standpoint: the majority of page.screenshot usages have path option in it.

it's cheap from the project standpoint.

aslushnikov · 2017-09-29T19:03:59Z

lib/helper.js

+   * @param {function(?)} nodeFunction
+   * @return {function(?):!Promise<?>}
+   */
+  static promisify(nodeFunction) {


@graingert no need to pull in a dependency for 30 lines of code.

aslushnikov · 2017-09-29T19:04:15Z

lib/helper.js

+    if (typeof nodeFunction !== 'function')
+      throw new TypeError('A function is expected');
+
+    // Because native util.promisify differently work with arguments


nit: let's drop the comment

aslushnikov · 2017-09-29T19:07:15Z

lib/helper.js

+      });
+    }
+
+    promisified.__isPromisified__ = true;


this is never used - let's drop it.

aslushnikov · 2017-09-29T19:07:36Z

lib/helper.js

+          if (err)
+            return reject(err);
+
+          // If callback invoked with single argument, we return in in Promise directly


nit: let's drop the comment, the code is self explanatory

aslushnikov · 2017-09-29T19:09:53Z

lib/helper.js

+   * @return {function(?):!Promise<?>}
+   */
+  static promisify(nodeFunction) {
+    if (typeof nodeFunction !== 'function')


Let's drop the check - the API is for internal use only.

aslushnikov · 2017-09-29T19:13:45Z

lib/Launcher.js

+        if (temporaryUserDataDir) {
+          removeFolderAsync(temporaryUserDataDir)
+              .then(() => fulfill())
+              .catch(err => console.error(err));


@graingert to avoid unhandledPromiseRejection.

aslushnikov · 2017-09-29T19:19:01Z

lib/Launcher.js

-          removeSync(temporaryUserDataDir);
-        fulfill();
+        if (temporaryUserDataDir) {
+          removeFolderAsync(temporaryUserDataDir)


@CheerlessCloud I've seen your comment that the promise is not not catched in a few places, thanks for the clarification.

let's fulfill regardless of the temprorary data directory cleanup, and use debugError instead of console.error:

if (temporaryUserDataDir) { removeFolderAsync(temporaryUserDataDir) .catch(debugError) .then(fulfill); } else { fulfill(); }

I want to be able to try/catch the error that removing the temporaryUserDataDir throws. You should await it and throw.

@graingert what are you going to do with the exception? The temporaryUserDataDir is internal to launcher's implementation.

graingert · 2017-09-29T19:24:04Z

lib/Launcher.js

-          removeSync(temporaryUserDataDir);
-        fulfill();
+        if (temporaryUserDataDir) {
+          removeFolderAsync(temporaryUserDataDir)


I want to be able to try/catch the error that removing the temporaryUserDataDir throws. You should await it and throw.

aslushnikov · 2017-10-03T02:52:16Z

@CheerlessCloud any updates on this? I'm very interested in this going in.

CheerlessCloud · 2017-10-03T10:56:24Z

@aslushnikov Sorry, I've been busy the last few days. I'll try to finish it in the near future)

aslushnikov · 2017-10-10T00:23:11Z

Hey @CheerlessCloud!

I see you're pressed on time. I'd like to add a few commits to this PR to address all the comments and merge this in. Let me know if this would work for you.

@JoelEinbinder

I failed to come up with a proper jsdoc for the method. @JoelEinbinder.

This patch: - introduces `helper.promisify` - a simple polyfill for the `util.promisify`. The `util.promisify` could not be used due to Node6 compatibility issues. - migrates all sync filesystem operations to the async replicas Fixes puppeteer#884.

[fix](page): Replaced writeFileSync by asynchronous version in Page.js

759889b

paulirish mentioned this pull request Sep 26, 2017

What is the reason of using a sync version of writeFile for pdf save? #884

Closed

Garbee approved these changes Sep 26, 2017

View reviewed changes

ebidel reviewed Sep 26, 2017

View reviewed changes

CheerlessCloud added 3 commits September 27, 2017 08:10

[feat](helper/promisify): Added 'promisify' method to helper.

9420ef0

[feat](helper/promisifyAll): Added 'promisifyAll' method to Helper.

608efaa

[fix](page): Replaced promise wrapper for make "writeFileAsync" funct…

8570873

…ion by method from helper.

CheerlessCloud changed the title ~~[fix](page): Replaced writeFileSync by asynchronous version in Page.js~~ [fix]: Removed synchronous operation with file system. Sep 27, 2017

CheerlessCloud added 2 commits September 27, 2017 11:22

[fix](launcher): Removed synchronous operation with file system.

1889b83

[fix](tracing): Removed synchronous operation with file system.

fc34f65

CheerlessCloud force-pushed the master branch from 13eaf6d to fc34f65 Compare September 27, 2017 08:23

aslushnikov reviewed Sep 27, 2017

View reviewed changes

CheerlessCloud added 3 commits September 27, 2017 14:01

[refactor](helper/promisify): Remove support for util.promisify.custo…

fbe8598

…m and delete promisifyAll method.

[refactor](launcher): Refactor some code.

ca8084c

Rename rimrafAsync to removeFolderAsync. Place mkdtempAsync locally. Use removeFolder.sync in killChrome function.

[refactor](tracing): Moved async verions of fs methods to local scope.

b7d2caf

graingert reviewed Sep 28, 2017

View reviewed changes

graingert suggested changes Sep 28, 2017

View reviewed changes

aslushnikov reviewed Sep 29, 2017

View reviewed changes

graingert suggested changes Sep 29, 2017

View reviewed changes

aslushnikov mentioned this pull request Oct 10, 2017

feat(Frame): Support options in addScriptTag and addStyleTag #996

Merged

aslushnikov added 4 commits October 11, 2017 00:23

Merge branch 'master' into master

d483726

Fix bad merge for the Launcher.js

1d961ae

Address comments for lib/helper.js

ea5c905

Remove jsdoc for helper.promisify

6d15383

I failed to come up with a proper jsdoc for the method. @JoelEinbinder.

aslushnikov merged commit ff08e45 into puppeteer:master Oct 11, 2017

		const ChromiumRevision = require('../package.json').puppeteer.chromium_revision;

		fs.mkdtempAsync = helper.promisify(fs.mkdtemp);

[fix]: Removed synchronous operation with file system. #879

[fix]: Removed synchronous operation with file system. #879

Conversation

CheerlessCloud commented Sep 26, 2017

googlebot commented Sep 26, 2017

CheerlessCloud commented Sep 26, 2017

googlebot commented Sep 26, 2017

Garbee 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

aslushnikov commented Sep 27, 2017 • edited

CheerlessCloud commented Sep 27, 2017

aslushnikov commented Sep 27, 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

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

CheerlessCloud Sep 27, 2017 • edited

Choose a reason for hiding this comment

petrgrishin commented Sep 28, 2017

graingert Sep 28, 2017 • edited

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

graingert Sep 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslushnikov commented Oct 3, 2017

CheerlessCloud commented Oct 3, 2017

aslushnikov commented Oct 10, 2017

aslushnikov commented Sep 27, 2017 •

edited

CheerlessCloud Sep 27, 2017 •

edited

graingert Sep 28, 2017 •

edited

graingert Sep 29, 2017 •

edited