Closed Bug 784717 Opened 7 years ago Closed 7 years ago

[OS.File] Implement asynchronous directory walk

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(3 files, 5 obsolete files)

No description provided.
Attached patch Companion testsuite (obsolete) — Splinter Review
Assignee: nobody → dteller
Attachment #655289 - Flags: review?(nfroyd)
Attached patch Worker code (obsolete) — Splinter Review
Attachment #655291 - Flags: review?(nfroyd)
Attached patch Controller code (obsolete) — Splinter Review
Attachment #655292 - Flags: review?(nfroyd)
Note to Nathan: to understand these patches, you will need to get start with the dependencies.
Comment on attachment 655292 [details] [diff] [review]
Controller code

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

Async programming makes my head hurt, but I think everything in here looks sound.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +600,5 @@
> +   * @resolves {OS.File.Entry}
> +   * @rejects {StopIteration} If all entries have already been visited.
> +   */
> +  next: function next() {
> +    let self = this;

I think it is a little more JavaScript-y to use .bind(this) on functions, but I will leave this up to you.  Certainly .bind can get tedious (e.g. your implementation of forEach)...

@@ +608,5 @@
> +    promise = promise.then(
> +      function withIterator(iterator) {
> +        return self._next(iterator);
> +      }
> +    );

Nit: I think the prevailing style is to });, rather than a more C-like brace style.  Please change this and other instances.  (And note that you were not completely consistent ;)

@@ +625,5 @@
> +  nextBatch: function nextBatch(size) {
> +    if (this._isClosed) {
> +      return Promise.resolve(null);
> +    }
> +    let self = this;

self is not needed here.

@@ +636,5 @@
> +    promise = promise.then(
> +      function withEntries(array) {
> +        if (array == null) {
> +          return null;
> +        }

I remember saying that nextBatch should return an array always, even if it was a zero-length array.  Please remove this null check.

I think that means the ._isClosed check above should return Promise.resolve([]), yes?
Attachment #655292 - Flags: review?(nfroyd) → feedback+
Comment on attachment 655291 [details] [diff] [review]
Worker code

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +159,5 @@
> +       }
> +       if (!(file instanceof File.DirectoryIterator)) {
> +         throw new Error("file is not a directory iterator " + file.__proto__.toSource());
> +       }
> +       return f.call(file);

I am amused that none of the functions you pass to withDir actually name their parameter.  I think this is intentional, but is the |file| parameter going to be useful further down the road?

@@ +203,5 @@
>         open: function open(path, mode, options) {
>           let file = File.open(Type.path.fromMsg(path), mode, options);
>           return OpenedFiles.add(file);
>         },
> +       new_DirectoryIterator: function(path, options) {

Please give this function a name.

@@ +265,5 @@
> +               }
> +               throw x;
> +             }
> +           }
> +         );

Nit: });  And other places.

@@ +280,5 @@
> +             }
> +             if (result == null) {
> +               // We have reached the end
> +               OpenedDirectoryIterators.remove(dir);
> +             }

nextBatch shouldn't be returning null.  And this null check fails to stop result.map from throwing an error.  Please fix.
Attachment #655291 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #5)
> I think it is a little more JavaScript-y to use .bind(this) on functions,
> but I will leave this up to you.  Certainly .bind can get tedious (e.g. your
> implementation of forEach)...

Both tedious, slow and error-prone, so for the moment, I am avoiding it until someone r- my code because of this:)

> @@ +608,5 @@
> > +    promise = promise.then(
> > +      function withIterator(iterator) {
> > +        return self._next(iterator);
> > +      }
> > +    );
> 
> Nit: I think the prevailing style is to });, rather than a more C-like brace
> style.  Please change this and other instances.  (And note that you were not
> completely consistent ;)

Oops :)

> @@ +625,5 @@
> > +  nextBatch: function nextBatch(size) {
> > +    if (this._isClosed) {
> > +      return Promise.resolve(null);
> > +    }
> > +    let self = this;
> 
> self is not needed here.

Thanks.

> 
> @@ +636,5 @@
> > +    promise = promise.then(
> > +      function withEntries(array) {
> > +        if (array == null) {
> > +          return null;
> > +        }
> 
> I remember saying that nextBatch should return an array always, even if it
> was a zero-length array.  Please remove this null check.

Right, thanks.

> 
> I think that means the ._isClosed check above should return
> Promise.resolve([]), yes?

Indeed.
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Comment on attachment 655291 [details] [diff] [review]
> Worker code
> 
> Review of attachment 655291 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_async_worker.js
> @@ +159,5 @@
> > +       }
> > +       if (!(file instanceof File.DirectoryIterator)) {
> > +         throw new Error("file is not a directory iterator " + file.__proto__.toSource());
> > +       }
> > +       return f.call(file);
> 
> I am amused that none of the functions you pass to withDir actually name
> their parameter.  I think this is intentional, but is the |file| parameter
> going to be useful further down the road?

Well, there is no |file| parameter (|call|'s first argument sets |this|), so probably not :)


> @@ +203,5 @@
> >         open: function open(path, mode, options) {
> >           let file = File.open(Type.path.fromMsg(path), mode, options);
> >           return OpenedFiles.add(file);
> >         },
> > +       new_DirectoryIterator: function(path, options) {
> 
> Please give this function a name.
> 
> @@ +265,5 @@
> > +               }
> > +               throw x;
> > +             }
> > +           }
> > +         );
> 
> Nit: });  And other places.
> 
> @@ +280,5 @@
> > +             }
> > +             if (result == null) {
> > +               // We have reached the end
> > +               OpenedDirectoryIterators.remove(dir);
> > +             }
> 
> nextBatch shouldn't be returning null.  And this null check fails to stop
> result.map from throwing an error.  Please fix.

Ok.
Attached patch Controller code, v2 (obsolete) — Splinter Review
Attachment #655292 - Attachment is obsolete: true
Attachment #656045 - Flags: review?(nfroyd)
Attached patch Worker code, v2 (obsolete) — Splinter Review
Attachment #655291 - Attachment is obsolete: true
Attachment #656050 - Flags: review?(nfroyd)
Comment on attachment 656045 [details] [diff] [review]
Controller code, v2

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

This is a bit of a rubberstamp.  I assume that all the read/write stuff will need updating for the options.bytes changes made elsewhere, correct?

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +50,5 @@
> +
> +/**
> + * An implementation of queues (FIFO).
> + *
> + * The current implementation uses two arrays and runs in O(n * log(n)).

n log n?  Really?  No copies, no recursive calls, what is driving the log n factor here?  I'm not going to nitpick about this, but I find this very surprising.
Attachment #656045 - Flags: review?(nfroyd) → review+
Comment on attachment 656050 [details] [diff] [review]
Worker code, v2

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +286,2 @@
>             }
>           );

Feel free to }); this. :)
Attachment #656050 - Flags: review?(nfroyd) → review+
Attachment #655289 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #11)
> Comment on attachment 656045 [details] [diff] [review]
> Controller code, v2
> 
> Review of attachment 656045 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a bit of a rubberstamp.  I assume that all the read/write stuff will
> need updating for the options.bytes changes made elsewhere, correct?

Indeed. All part of bug 769191 and bug 777711.

> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +50,5 @@
> > +
> > +/**
> > + * An implementation of queues (FIFO).
> > + *
> > + * The current implementation uses two arrays and runs in O(n * log(n)).
> 
> n log n?  Really?  No copies, no recursive calls, what is driving the log n
> factor here?  I'm not going to nitpick about this, but I find this very
> surprising.

Array growth. I have not actually checked the implementation of array growth in SpiderMonkey, but the usual implementation is O(n * log(n)).
Summary: Implement asynchronous directory walk → [OS.File] Implement asynchronous directory walk
Attachment #655289 - Attachment is obsolete: true
Attachment #665191 - Flags: review+
Attached patch Worker code, v2Splinter Review
Attachment #656050 - Attachment is obsolete: true
Attachment #665192 - Flags: review+
Attachment #656045 - Attachment is obsolete: true
Attachment #665193 - Flags: review+
You need to log in before you can comment on or make changes to this bug.