938704 - Make OS.File support ES6 iterators

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Brandon Benvie [:benvie]]

Right now OS.File supports the legacy __iterator__ that overloads for-in and also uses StopIteration. OS.File should be upgraded to use ES6 style iterators which use @@iterator and return `{ done, value }` on each iteration, rather than using StopIteration.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

For reference, here's a guide to @@iterator: http://domenic.me/2013/09/06/es6-iterators-generators-and-iterables/

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Looks like we're going to make some unhappy clients. The __iterator__ + StopIteration played nicely with throwing StopIteration from a generator, this new version doesn't seem so accommodating.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Ok, it's officially worse: __iterator__ supports |for in|, @@iterator supports |for of|.
I'll probably need to add a transitional __iterator__ wrapper.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.File to @@iterator

Here we go.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion tests

And the tests.
I took the opportunity to remove the async DirectoryIterator tests from mochi/ and into xpcshell/.

Comment 6

[Brandon Benvie [:benvie]]

Comment on attachment 8337968 [details] [diff] [review]
Porting OS.File to @@iterator

Review of attachment 8337968 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good! A few fixes and nits.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +921,2 @@
>     */
> +  next: function() {

Nit: `next: function next() {` maybe? To follow the style of rest of the methods.

@@ +928,5 @@
> +      if (done) {
> +        this.close();
> +        return this._itmsg;
> +      }
> +      return value;

This should be `return DirectoryIterator.Entry.fromMsg(value)`.

@@ +929,5 @@
> +        this.close();
> +        return this._itmsg;
> +      }
> +      return value;
> +    }.this);

`.bind(this)`, or use an arrow function.

@@ +981,2 @@
>      let position = 0;
> +    return Task.spawn(function() {

Should probably use function* here.

@@ +987,5 @@
> +                yield Scheduler.post("DirectoryIterator_prototype_next", [iterator]);
> +          if (done) {
> +            break;
> +          }
> +          yield cb(value, position++, this);

Also need `DirectoryIterator.Entry.fromMsg(value)` here.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting clients

Comment 8

[David Teller [:Yoric] - still alive but not very active]

(In reply to Brandon Benvie [:benvie] from comment #6)
> This should be `return DirectoryIterator.Entry.fromMsg(value)`.

Ah, right. Since that's basically identity, I would have missed that.

> @@ +929,5 @@
> > +        this.close();
> > +        return this._itmsg;
> > +      }
> > +      return value;
> > +    }.this);
> 
> `.bind(this)`, or use an arrow function.

Wow. I'm shocked that this didn't throw a SyntaxError.

> @@ +981,2 @@
> >      let position = 0;
> > +    return Task.spawn(function() {
> 
> Should probably use function* here.

I thought that function* was disabled for the moment?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.File to @@iterator, v2

Applied benvie's feedback.

Comment 10

[Brandon Benvie [:benvie]]

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #8)
> I thought that function* was disabled for the moment?

I believe it was disabled for aurora but that was a temporary thing (not sure if they're still disabled on Aurora or not). It's being used in a number of places in m-c now (http://mxr.mozilla.org/mozilla-central/search?string=function*&find=\.jsm*&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central).

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8338036 [details] [diff] [review]
Porting OS.File to @@iterator, v2

Review of attachment 8338036 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with fixing the Unix/Windows inconsistency.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +929,5 @@
> +        this.close();
> +        return this._itmsg;
> +      }
> +      return DirectoryIterator.Entry.fromMsg(value);
> +    }.bind(this));

Do we have fat arrow functions available nowadays?  We could probably dispense with .bind()s here and elsewhere.  Followup bug, perhaps?

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +698,5 @@
>         if (!this._exists) {
>           throw File.Error.noSuchFile("DirectoryIterator.prototype.next");
>         }
>         if (this._closed) {
> +         return {done: true, value: undefined};

We have disagreements between the Unix and Windows versions about what |value| is when we're done (undefined here, null for windows).  For consistency's sake, we should make them the same.

Alternatively, we could have AbstractIterator define a DONE_OBJECT or somesuch that we could always use in these situations.

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8337970 [details] [diff] [review]
Companion tests

Review of attachment 8337970 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but why did you pull the test out to its own file and change the iteration details in a single patch?

Comment 13

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Comment on attachment 8338036 [details] [diff] [review]
> Porting OS.File to @@iterator, v2
> 
> Review of attachment 8338036 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with fixing the Unix/Windows inconsistency.
> 
> ::: toolkit/components/osfile/modules/osfile_async_front.jsm
> @@ +929,5 @@
> > +        this.close();
> > +        return this._itmsg;
> > +      }
> > +      return DirectoryIterator.Entry.fromMsg(value);
> > +    }.bind(this));
> 
> Do we have fat arrow functions available nowadays?  We could probably
> dispense with .bind()s here and elsewhere.  Followup bug, perhaps?

I don't really like fat arrow functions for multiline functions. If you wish, I can file a followup bug nevertheless, though.

> 
> ::: toolkit/components/osfile/modules/osfile_unix_front.jsm
> @@ +698,5 @@
> >         if (!this._exists) {
> >           throw File.Error.noSuchFile("DirectoryIterator.prototype.next");
> >         }
> >         if (this._closed) {
> > +         return {done: true, value: undefined};
> 
> We have disagreements between the Unix and Windows versions about what
> |value| is when we're done (undefined here, null for windows).  For
> consistency's sake, we should make them the same.
> 
> Alternatively, we could have AbstractIterator define a DONE_OBJECT or
> somesuch that we could always use in these situations.

Actually, the value is ignored, so it's not a big deal, but let's make it |undefined|.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.File to @@iterator, v3

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=b8602e81e8f3

Comment 15

[Tom S. (please needinfo tschuster)]

Can somebody commit to working on this? As far as I can tell this is the biggest hurdle to removing __iterator__, which causes ES6 problems.

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 938704 - OS.File + ES6 - WIP

Review commit: https://reviewboard.mozilla.org/r/23917/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/23917/

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Looks like I won't have time to work on this, so I have pushed a WIP. If my memory serves, all of OS.File is ported to ES6 iterators, but not all client code. If anybody wants to take this forward, please be my guest.

Comment 18

[Tooru Fujisawa [:arai]]

bug 1331092 (and async iteration protocol) may be interesting for this case, given that this is asynchronous iteration.

Comment 19

[Tom S. (please needinfo tschuster)]

David, is there anybody on the Firefox team who could work on this?

Comment 20

[David Teller [:Yoric] - still alive but not very active]

I suspect that Paolo would know better than me.

Comment 21

[:Paolo Amadini]

At present most of us are busy with Firefox 57, but it's something we could work on afterwards, if this is the question :-)

In some cases, we may have an opportunity for using async generators as well. We should also wait a few releases to do that as they're a pretty new feature in our JavaScript engine.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Bug 938704 - Make OS.File support modern iterators.

Review commit: https://reviewboard.mozilla.org/r/170494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170494/

Comment 23

[Masatoshi Kimura [:emk]]

We don't have to care about addon-compat anymore.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review175824

That looks like a nice simplification, thanks!

::: toolkit/components/osfile/modules/osfile_async_front.jsm
(Diff revision 1)
> -      return Promise.resolve();
> +      return undefined;
>      }
>      this._isClosed = true;
> -    let self = this;
> -    return this._itmsg.then(
> +    let iterator = this._itmsg;
> +    this._itmsg = null;
> -      function withIterator(iterator) {

I believe that the comment was useful. Could you keep/adapt it?

::: toolkit/components/osfile/modules/osfile_async_worker.js:366
(Diff revision 1)
> -         try {
> -           return File.DirectoryIterator.Entry.toMsg(this.next());
> +         let {value, done} = this.next();
> +         if (done) {
> -         } catch (x) {
> -           if (x == StopIteration) {
> -             OpenedDirectoryIterators.remove(dir);
> +           OpenedDirectoryIterators.remove(dir);
> +           return { done: true };

For (shape) consistency, shouldn't you return

```js
{ value: undefined, done: true }
```

and

```js
{ value: File.DirectoryIterator.Entry.toMsg(value), done: false }
```

?

Also, the same wherever you return `{ value: ... }` or `{done: ...}`.

::: toolkit/components/search/nsSearchService.js:2981
(Diff revision 1)
>          distDirs.push(dir);
>        } catch (ex) {
> -        // Catch for StopIteration exception.
>          if (ex.result == Cr.NS_ERROR_ALREADY_INITIALIZED) {
>            throw ex;
>          }

Mmmh... that looks weird. I realize it already looked weird before your patch, but someone should probably audit these lines.

::: toolkit/components/search/nsSearchService.js
(Diff revision 1)
>        try {
>          // Add dir to otherDirs if it contains any files.
>          await checkForSyncCompletion(iterator.next());
>          otherDirs.push(dir);
>        } catch (ex) {
> -        // Catch for StopIteration exception.

Same here.

Comment 25

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review175824

> I believe that the comment was useful. Could you keep/adapt it?

Since we no longer lazyly initialize a rejected promise and now we always chack isClosed before touching _itmsg, I'm not sure we have anything to note here.

> For (shape) consistency, shouldn't you return
> 
> ```js
> { value: undefined, done: true }
> ```
> 
> and
> 
> ```js
> { value: File.DirectoryIterator.Entry.toMsg(value), done: false }
> ```
> 
> ?
> 
> Also, the same wherever you return `{ value: ... }` or `{done: ...}`.

Done.

> Mmmh... that looks weird. I realize it already looked weird before your patch, but someone should probably audit these lines.

Indeed the previous version did not keep the semantics, thanks.

Comment 26

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170494/diff/1-2/

Comment 27

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review175912

::: commit-message-5ca56:1
(Diff revision 2)
> +Bug 938704 - Make OS.File support modern iterators. r?Yoric,florian

Florian, could you double-check the nsSearchService.js change?

Comment 28

[Florian Quèze [:florian]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review175962

::: .eslintignore:359
(Diff revision 2)
>  toolkit/modules/AppConstants.jsm
>  toolkit/mozapps/downloads/nsHelperAppDlg.js
>  toolkit/mozapps/update/tests/data/xpcshellConstantsPP.js
>  
> +# Uses for-await-of syntax
> +toolkit/mozapps/extensions/internal/XPIProvider.jsm

Not linting the whole file for a problem on a single line doesn't seem great. Can we use // eslint-disable-line instead?

::: toolkit/components/search/nsSearchService.js:2980
(Diff revision 2)
>          // Add dir to distDirs if it contains any files.
> -        await checkForSyncCompletion(iterator.next());
> +        let {done} = await checkForSyncCompletion(iterator.next());
> +        if (!done) {
> -        distDirs.push(dir);
> +          distDirs.push(dir);
> +        }
>        } catch (ex) {

Do we still need this catch block?

::: toolkit/components/search/nsSearchService.js:3006
(Diff revision 2)
>          // Add dir to otherDirs if it contains any files.
> -        await checkForSyncCompletion(iterator.next());
> +        let {done} = await checkForSyncCompletion(iterator.next());
> +        if (!done) {
> -        otherDirs.push(dir);
> +          otherDirs.push(dir);
> +        }
>        } catch (ex) {

Same question here.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6570
(Diff revision 2)
>        logger.error("Failed to clean updated system add-ons directories.", e);
>        return;
>      }
>  
>      try {
> -      for (let promise in iterator) {
> +      for await (let entry of iterator) {

Are we sure it's OK to use 'for await'? https://groups.google.com/forum/#!topic/mozilla.dev.platform/J1cSMVqwZAY seems to say we should wait a cycle or two.

Comment 29

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review175962

> Not linting the whole file for a problem on a single line doesn't seem great. Can we use // eslint-disable-line instead?

ESLint does not support the for-await-of syntax at all yet. It does not even parse this file as a valid JavaScript file. So I had no choice.

That said, it does not matter anymore because I retracted the use of for-await-of. (See below.)

> Do we still need this catch block?

Yes. checkForSyncCompletion may throw NS_ERROR_ALREADY_INITIALIZED. (The old comment was a bit misleading.)

> Are we sure it's OK to use 'for await'? https://groups.google.com/forum/#!topic/mozilla.dev.platform/J1cSMVqwZAY seems to say we should wait a cycle or two.

Hmm, I had to re-introduce `__iterator__` and manually implement the async iterator protocol at the moment :(

Comment 30

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170494/diff/2-3/

Comment 31

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170494/diff/2-3/

Comment 32

[Florian Quèze [:florian]]

(In reply to Masatoshi Kimura [:emk] from comment #29)

> > Do we still need this catch block?
> 
> Yes. checkForSyncCompletion may throw NS_ERROR_ALREADY_INITIALIZED. (The old
> comment was a bit misleading.)

We specifically don't want to catch NS_ERROR_ALREADY_INITIALIZED, that's why we throw it again if it's the exception that got caught. I don't think the old comment was misleading.

Comment 33

[Masatoshi Kimura [:emk]]

Hm, the old code used to swallow all exceptions but NS_ERROR_ALREADY_INITIALIZED. It should have been?
  if (ex != StopIteration) {
?

Comment 34

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170494/diff/3-4/

Comment 35

[Tooru Fujisawa [:arai]]

as long as DirectoryIterator.prototype implements async iteration protocol, you can iterate over it manually with something like the following, without adding __iterator__.

let asyncIterator = Symbol.asyncIterator || "@@asyncIterator";
let DirectoryIterator.prototype = {
  [asyncIterator]() {
    return this;
  },
  ...
};

let iter = iterator[asyncIterator]();
try {
  for (;;) {
    let {value: entry, done} = await iter.next();
    if (done) {
      iter = null;
      break;
    }
    /* loop body here */
  }
} finally {
  if (iter && "return" in iter && typeof iter.return == "function") {
    iter.return();
  }
}

Comment 36

[Tooru Fujisawa [:arai]]

also, in that case iterator.close call can be stored into iterator.return, that will be called automatically if you switch to for-await-of.

> let DirectoryIterator.prototype = {
s/let //

Comment 37

[Masatoshi Kimura [:emk]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170494/diff/4-5/

Comment 38

[Masatoshi Kimura [:emk]]

Instead of strictly adhering the async iterator protocol, I used the knowledge of implemention details of DirectoryIterator. For example, I omitted iterator[Symbol.asyncIterator]() because it will just return itself.

Comment 39

[Florian Quèze [:florian]]

Comment on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

https://reviewboard.mozilla.org/r/170494/#review176350

Looks good to me, thanks!

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f2356284595
Make OS.File support modern iterators. r=florian,Yoric

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7f2356284595