Async/await refactoring for files in io folder except directory.js #1697

hameleonka · 2017-12-12T03:16:04Z

@rpl in this PR I refactored files async/await syntax in io folder excluding directory.js (currently in work by another contributor). Please check it out, waiting for your review.

asamuzaK · 2017-12-13T01:08:58Z

node.js v6 does not support async/await.
Is there a decision somewhere that linter is going to drop v6 support?

EnTeQuAk · 2017-12-13T01:23:20Z

@asamuzaK no, we won't drop support for v6 anytime soon, at least nothing like that is planned at this moment. This is a follow-up for #1692 which allows our babel stack to transpile async/await so that we should stay compatible with node6.

asamuzaK · 2017-12-13T01:25:17Z

Got it, thanks.

rpl · 2017-12-20T13:36:19Z

src/io/xpi.js

@@ -33,7 +33,7 @@ export class Xpi extends IOBase {
    });
  }

-  handleEntry(entry, reject) {
+  handleEntry(entry) {


This method still need to get the reject parameter from its callers and to use it to reject the error at line 46 (the DuplicateZipEntry) instead of throwing it as a synchronously raised exception.

The reason is the following:

handleEntry is called from inside an EventEmitter event handler, which is subscribed from inside the Promise returned by the getFiles method (in both the "src/io/xpi.js" and "src/io/crx.js" modules)

the behavior that we expect from the promise returned from getFiles is:

to be resolved with the list of the file entries once the zipFile and no duplicate entries have been found

to be rejected as soon as handleEntry is called with a duplicate entry filename

Given that handleEntry is called from inside an event handler, if it throws an exception instead of calling reject, the raised exception is not going to be translated into a rejection of the Promise returned by getFiles, on the contrary it is going to become an "uncaught exception" (which in nodejs can be caught using process.on("uncaughtException", (err) => { ... });).

We definitely need to preserve the previous behavior, which is that the rejection will be caught by the Linter class in one of the following this.handleError(...) calls:

addons-linter/src/linter.js

Line 479 in 2025658

this.handleError(err, deps._console);

addons-linter/src/linter.js

Line 499 in 2025658

this.handleError(err, deps._console);

Also, based on this analysis, the very weird (and concerning) thing is that the different behavior doesn't seem to trigger any test failures, which basically means that something in the current testing strategy is making the tests to behave differently from what is going to happen in the application once executed for real, which is pretty bad and we need to fix it.

After digging a bit into the code and the test suite, I've been able to identify what is affecting the behavior for the tests related to this scenario (related to the DuplicateZipEntry):

In the test cases we are using the _onEventsSubscribed parameter of getFiles to be able to "fake" the zip file entries when the zipfile listeners have been subscribed, see:

https://github.com/hameleonka/addons-linter/blob/6fd67e426da1130e41feb8c3d5a1c529af71129a/tests/io/test.xpi.js#L215-L219

and

https://github.com/hameleonka/addons-linter/blob/6fd67e426da1130e41feb8c3d5a1c529af71129a/src/io/xpi.js#L84-L90

But _onEventsSubscribed is called synchronously at line 88 of the above snippet, and in the test code linked above the definition of _onEventsSubscribed is calling synchronously the this.handleEntry method, which had the (unintentional) side-effect that we are experiencing: an Error raised by the new version of the handleEntry method is becoming a rejction of the promise returned by getFiles... but only in the test cases that are using _onEventsSubscribed in this way.

That's very unfortunate and we should absolutely prevent it from happening again (so that we can be more confident that the test suite is going to be able to catch similar regressions), my suggestion is to change the way we call the _onEventSubscribed callback, so that it is not called synchronously from inside the Promise, and better match what happens when this.handleEntry is called from inside the zipfile listeners, e.g. by chaging it into:

if (_onEventsSubscribed) { // Run optional callback when we know the event handlers // have been inited. Useful for testing. if (typeof _onEventsSubscribed === 'function') { Promise.resolve().then(() => _onEventsSubscribed(reject)); } }

@rpl as you suggested in order to see the difference between throw error and reject I put throw error in the beginning of handleEntry function and saw that with reject this error is handled with handleError function and without reject node process just fails. Thanks for your help!

rpl · 2017-12-20T13:37:27Z

src/io/xpi.js

@@ -43,53 +43,51 @@ export class Xpi extends IOBase {
    }
    if (this.entries.includes(entry.fileName)) {
      log.info('Found duplicate file entry: "%s" in package', entry.fileName);
-      reject(new Error(oneLine`DuplicateZipEntry: Entry
-        "${entry.fileName}" has already been seen`));
+      throw new Error(oneLine`DuplicateZipEntry: Entry


besides changing this in a reject(new Error(...)), we should probably also return early just after calling reject(...).

rpl · 2017-12-20T13:38:29Z

src/io/xpi.js

-        .catch(reject);
+    const zipfile = await this.open();
+
+    return new Promise((resolve) => {


(resolve, reject) => {...} here for the reasons described in the previous comment.

(same for "src/io/crx.js")

rpl · 2017-12-20T13:39:21Z

src/io/xpi.js

+
+    return new Promise((resolve) => {
+      zipfile.on('entry', (entry) => {
+        this.handleEntry(entry);


this.handleEntry(entry, reject); here as previously described (same for "src/io/crx.js").

I'm also wondering if we should also listen to zipfile.on('error', ...)` and call reject also in this case.

@rpl do I need to add zipfile.on('error', ...)` and tests for this in this PR or we should open new issue for this?

rpl · 2017-12-20T13:41:10Z

src/io/xpi.js

+        // Run optional callback when we know the event handlers
+        // have been inited. Useful for testing.
+        if (typeof _onEventsSubscribed === 'function') {
+          _onEventsSubscribed();


Promise.resolve().then(() => _onEventsSubscribed(reject)); for the reasons previously described.

(same for "src/io/crx.js")

rpl · 2017-12-20T13:46:39Z

src/io/xpi.js

-          zipfile.openReadStream(this.files[path], (err, readStream) => {
-            if (err) {
-              return reject(err);
+      // eslint-disable-next-line consistent-return


We should be able to remove this eslint-disable-next-line here by changing line 143 into:

if (err) { reject(err); return; }

rpl

just some minor tweaks

rpl · 2017-12-22T19:20:48Z

src/io/crx.js

@@ -76,7 +76,7 @@ export class Crx extends Xpi {
        // Run optional callback when we know the event handlers
        // have been inited. Useful for testing.
        if (typeof _onEventsSubscribed === 'function') {
-          _onEventsSubscribed();
+          Promise.resolve().then(() => _onEventsSubscribed(reject));


passing reject as a parameter of _onEventsSubscribed doesn't look necessary and none of the tests are currently expecting any parameter passed to this function, eg.

addons-linter/tests/io/test.xpi.js

Lines 215 to 219 in 2025658

const onEventsSubscribed = () => {

const entryCallback = this.entryStub.firstCall.args[1];

entryCallback.call(null, installFileEntry);

entryCallback.call(null, dupeInstallFileEntry);

};

rpl · 2017-12-22T19:21:50Z

src/io/xpi.js

@@ -85,7 +86,7 @@ export class Xpi extends IOBase {
        // Run optional callback when we know the event handlers
        // have been inited. Useful for testing.
        if (typeof _onEventsSubscribed === 'function') {
-          _onEventsSubscribed();
+          Promise.resolve().then(() => _onEventsSubscribed(reject));


passing reject doesn't seem to be necessary (as from the same comment for crx.js)

rpl · 2017-12-26T15:18:57Z

src/io/utils.js

@@ -14,32 +14,27 @@ export function walkPromise(curPath, { shouldIncludePath = () => true } = {}) {
  // so all file paths (the result keys) can
  // be relative to the starting point.
  const basePath = curPath;
+  // eslint-disable-next-line consistent-return


If I'm reading this walk function correctly, we should be able to remove this eslint-disable-next-line comment by remove the return result from line 24 and 37 and add a single return result outsitde of the if (...) {...} else if (...) ....

(I'm also wondering if we should raise an error if none of the conditions are met)

@rpl I made one return statement after if-else block, but I don't know about raising an error...As I understand we check if it's file and if it's directory... but what else could it be?

When the files are coming directly from the filesystem (instead of coming from the content of a zip file, e.g. for an xpi or crx file), the stat result could also be a block or character device, or a FIFO or a socket (based on the related nodejs docs: https://nodejs.org/api/fs.html#fs_class_fs_stats), which is unexpected but still possible.

Currently we just silently ignore these files, and so I'm wondering if it would be better to produce a warning or an error to make it more visible to the addon developer.

Anyway, I'm also ok to defer this to a follow up (so that we keep the changes related to the async/await refactoring separated from changes of the addons-linter behaviors).

@EnTeQuAk what do you think about this?

refactors io except directory.js

6fd67e4

rpl self-requested a review December 19, 2017 13:34

rpl requested changes Dec 20, 2017

View reviewed changes

rpl added the state:in_progress label Dec 20, 2017

reverses changes to handleEntry function, fixes tests callback call

3d222df

rpl requested changes Dec 26, 2017

View reviewed changes

changes onEventSubscribed call and walkPromise function

70201ca

rpl approved these changes Jan 3, 2018

View reviewed changes

rpl added state:pull_request_ready and removed state:in_progress labels Jan 3, 2018

EnTeQuAk merged commit 3e8e274 into mozilla:master Jan 3, 2018

data-sync-user mentioned this pull request Oct 12, 2023

chore(deps-dev): bump node-fetch from 2.6.7 to 3.3.1 #4752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async/await refactoring for files in io folder except directory.js #1697

Async/await refactoring for files in io folder except directory.js #1697

hameleonka commented Dec 12, 2017

asamuzaK commented Dec 13, 2017

EnTeQuAk commented Dec 13, 2017

asamuzaK commented Dec 13, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl Dec 20, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl Dec 20, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl Dec 20, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl Dec 20, 2017

hameleonka Dec 21, 2017

rpl left a comment

rpl Dec 22, 2017

hameleonka Dec 26, 2017

rpl Dec 22, 2017

hameleonka Dec 26, 2017

rpl Dec 26, 2017

hameleonka Dec 26, 2017

rpl Jan 3, 2018

	const onEventsSubscribed = () => {
	const entryCallback = this.entryStub.firstCall.args[1];
	entryCallback.call(null, installFileEntry);
	entryCallback.call(null, dupeInstallFileEntry);
	};

Async/await refactoring for files in io folder except directory.js #1697

Async/await refactoring for files in io folder except directory.js #1697

Conversation

hameleonka commented Dec 12, 2017

asamuzaK commented Dec 13, 2017

EnTeQuAk commented Dec 13, 2017

asamuzaK commented Dec 13, 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

Choose a reason for hiding this comment

rpl 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