Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Oct 11, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 12 additions & 7 deletions lib/Launcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const childProcess = require('child_process');
const Downloader = require('../utils/ChromiumDownloader');
const Connection = require('./Connection');
const Browser = require('./Browser');
const readline = require('readline');
const fs = require('fs');
const {helper} = require('./helper');
const ChromiumRevision = require('../package.json').puppeteer.chromium_revision;

fs.mkdtempAsync = helper.promisify(fs.mkdtemp);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not spoil the global:

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


const CHROME_PROFILE_PATH = path.join(os.tmpdir(), 'puppeteer_dev_profile-');

const DEFAULT_ARGS = [
Expand Down Expand Up @@ -59,7 +61,7 @@ class Launcher {
const chromeArguments = [].concat(DEFAULT_ARGS);
if (!options.args || !options.args.some(arg => arg.startsWith('--user-data-dir'))) {
if (!options.userDataDir)
temporaryUserDataDir = fs.mkdtempSync(CHROME_PROFILE_PATH);
temporaryUserDataDir = await fs.mkdtempAsync(CHROME_PROFILE_PATH);

chromeArguments.push(`--user-data-dir=${options.userDataDir || temporaryUserDataDir}`);
}
Expand Down Expand Up @@ -90,8 +92,11 @@ class Launcher {
chromeProcess.once('close', () => {
// Cleanup as processes exit.
if (temporaryUserDataDir)
removeSync(temporaryUserDataDir);
fulfill();
rimrafAsync(temporaryUserDataDir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: braces are required around multi-line branches:

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

.then(() => fulfill())
.catch(err => console.error(err));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's reject the waitForChromeToClose promise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else
fulfill();
});
});

Expand All @@ -112,7 +117,7 @@ class Launcher {
/**
* @return {Promise}
*/
function killChrome() {
async function killChrome() {
helper.removeEventListeners(listeners);
if (temporaryUserDataDir) {
// Force kill chrome.
Expand All @@ -122,7 +127,7 @@ class Launcher {
process.kill(-chromeProcess.pid, 'SIGKILL');
// Attempt to remove temporary profile directory to avoid littering.
try {
removeSync(temporaryUserDataDir);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await rimrafAsync(temporaryUserDataDir);
} catch (e) { }
} else {
// Terminate chrome gracefully.
Expand Down
6 changes: 4 additions & 2 deletions lib/Page.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const {Keyboard, Mouse, Touchscreen} = require('./Input');
const Tracing = require('./Tracing');
const {helper, debugError} = require('./helper');

const writeFileAsync = helper.promisify(fs.writeFile);

class Page extends EventEmitter {
/**
* @param {!Session} client
Expand Down Expand Up @@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

return buffer;
}

Expand Down Expand Up @@ -633,7 +635,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);
return buffer;
}

Expand Down
12 changes: 8 additions & 4 deletions lib/Tracing.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
const fs = require('fs');
const {helper} = require('./helper');
const fs = require('fs');

fs.openAsync = helper.promisify(fs.open);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not spoil the globals

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

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

class Tracing {
/**
Expand Down Expand Up @@ -68,14 +72,14 @@ class Tracing {
*/
async _readStream(handle, path) {
let eof = false;
const file = fs.openSync(path, 'w');
const file = await fs.openAsync(path, 'w');
while (!eof) {
const response = await this._client.send('IO.read', {handle});
eof = response.eof;
if (path)
fs.writeSync(file, response.data);
await fs.writeAsync(file, response.data);
}
fs.closeSync(file);
await fs.closeAsync(file);
await this._client.send('IO.close', {handle});
}
}
Expand Down
81 changes: 81 additions & 0 deletions lib/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/

const debugError = require('debug')(`puppeteer:error`);
const util = require('util');
/** @type {?Map<string, boolean>} */
let apiCoverage = null;
class Helper {
Expand Down Expand Up @@ -225,6 +226,86 @@ class Helper {
static isNumber(obj) {
return typeof obj === 'number' || obj instanceof Number;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
      }
    });
  };
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and also the util.promisify method.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @static
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param {function(?)} nodeFunction
* @throws {TypeError} - Throws error if argument isn't a function.
* @return {function(?):!Promise<?>}
* @example
* const writeFileAsync = Helper.promisify(require('fs').writeFile);
* ...
* await writeFileAsync(...);
*/
static promisify(nodeFunction) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (typeof nodeFunction !== 'function')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new TypeError('A function is expected');


// If code run under NodeJS v8+, then by util.promisify.custom key
// may exist promisified version of function.
// see: https://nodejs.org/dist/latest-v8.x/docs/api/util.html#util_custom_promisified_functions
if (util.promisify && util.promisify.custom) {
if (nodeFunction[util.promisify.custom])
/**
* @param {!Array<?>} args
* @return {!Promise<?>}
*/
return util.promisify(nodeFunction);
}

// Because native util.promisify differently work with arguments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's drop the comment

// we can't use this function as ponyfill

/**
* @param {!Array<?>} args
* @return {!Promise<?>}
*/
function promisified(...args) {
return new Promise((resolve, reject) => {
function callback(err, ...result) {
if (err)
return reject(err);

// If callback invoked with single argument, we return in in Promise directly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (result.length === 1)
return resolve(result[0]);
else
// Otherwise return array of arguments
return resolve(result);
}
args.push(callback);
nodeFunction.call(null, ...args);
});
}

promisified.__isPromisified__ = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is never used - let's drop it.

return promisified;
}

/**
* @description Add to object for each method promisified analog by ${methodName}Async
* @static
* @param {Object} object
* @example
* const fs = Helper.promisifyAll(require('fs'));
* fs.writeFileAsync(...); // -> Promise
*/
static promisifyAll(object) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Yes, this method make promise wrapper for all methods in object,
// and for synchronous too.
// But because we don't rewrite method, just don't try use ${methodName}Async
// for initially synchronous methods.

Object.keys(object).forEach(key => {
if (typeof object[key] !== 'function')
return;

object[`${key}Async`] = Helper.promisify(object[key]);
});

return object;
}
}

module.exports = {
Expand Down