Closed Bug 938704 Opened 11 years ago Closed 7 years ago

Make OS.File support ES6 iterators

Categories

(Toolkit Graveyard :: OS.File, defect)

Product:
Toolkit Graveyard
Component:
OS.File
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bbenvie, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Async:team])

Attachments

(1 file, 6 obsolete files)

Bug 938704 - Make OS.File support modern iterators.
59 bytes, text/x-review-board-request
Yoric
: review+
florian
: review+
Details 
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.
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.
Keywords: addon-compat
Ok, it's officially worse: __iterator__ supports |for in|, @@iterator supports |for of|.
I'll probably need to add a transitional __iterator__ wrapper.
Attached patch Porting OS.File to @@iterator (obsolete) — — Splinter Review 
Here we go.
Assignee: nobody → dteller
Attachment #8337968 - Flags: review?(nfroyd)
Attachment #8337968 - Flags: review?(bbenvie)
Attached patch Companion tests (obsolete) — — Splinter Review 
And the tests.
I took the opportunity to remove the async DirectoryIterator tests from mochi/ and into xpcshell/.
Attachment #8337970 - Flags: review?(nfroyd)
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.
Attachment #8337968 - Flags: review?(bbenvie) → review+
(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?
Applied benvie's feedback.
Attachment #8337968 - Attachment is obsolete: true
Attachment #8337968 - Flags: review?(nfroyd)
Attachment #8338036 - Flags: review?(nfroyd)
(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 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.
Attachment #8338036 - Flags: review?(nfroyd) → review+
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?
Attachment #8337970 - Flags: review?(nfroyd) → review+
(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|.
Whiteboard: [Async] → [Async:team]
Blocks: 1127084
No longer blocks: 1127084
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.
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.
Assignee: dteller → nobody
bug 1331092 (and async iteration protocol) may be interesting for this case, given that this is asynchronous iteration.
David, is there anybody on the Firefox team who could work on this?
Flags: needinfo?(dteller)
I suspect that Paolo would know better than me.
Flags: needinfo?(dteller) → needinfo?(paolo.mozmail)
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.
Flags: needinfo?(paolo.mozmail)
We don't have to care about addon-compat anymore.
Assignee: nobody → VYV03354
Blocks: post-57-api-changes
Status: NEW → ASSIGNED
Keywords: addon-compat
Attachment #8703655 - Attachment is obsolete: true
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.
Attachment #8899232 - Flags: review?(dteller) → review+
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 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 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.
Attachment #8899232 - Flags: review?(florian)
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 :(
(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.
Hm, the old code used to swallow all exceptions but NS_ERROR_ALREADY_INITIALIZED. It should have been?
  if (ex != StopIteration) {
?
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();
  }
}
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 //
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 on attachment 8899232 [details]
Bug 938704 - Make OS.File support modern iterators.

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

Looks good to me, thanks!
Attachment #8899232 - Flags: review?(florian) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/7f2356284595
Make OS.File support modern iterators. r=florian,Yoric
https://hg.mozilla.org/mozilla-central/rev/7f2356284595
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: