Closed Bug 808492 Opened 7 years ago Closed 7 years ago

[OS.File] Support double-closing DirectoryIterator in asynchronous mode

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: Yoric, Assigned: mr_pathak)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 file, 5 obsolete files)

At the moment, closing a DirectoryIterator twice from the main thread yields an asynchronous error. We should fix this.
The simplest solution is probably to slightly alter function |withDir| in osfile_async_worker.js, and some of its callers, as follows:
- add an argument to |withDir| to determine whether the function should throw an error when the directory doesn't exist (currently behavior) or simply ignore the error and return when the directory doesn't exist (new behavior);
- in DirectoryIterator_prototype_close, adopt the new behavior.
Attached patch patch 0.1 (obsolete) — Splinter Review
Attachment #701011 - Flags: feedback?(dteller)
Comment on attachment 701011 [details] [diff] [review]
patch 0.1

@param {*} avl A variable determining whether to throw an error when the directory doesn't exist
Attachment #701011 - Attachment description: @param {*} avl A variable determining whether to throw an error when the directory doesn't exist → patch 0.1
Attached patch patch 0.2 (obsolete) — Splinter Review
Attachment #701011 - Attachment is obsolete: true
Attachment #701011 - Flags: feedback?(dteller)
Attachment #701020 - Flags: feedback?(dteller)
Comment on attachment 701020 [details] [diff] [review]
patch 0.2

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

Good start. Now let's complete this work – and add tests!

::: toolkit/components/osfile/osfile_async_worker.js
@@ +151,5 @@
>         return f.call(file);
>       };
>  
>       let OpenedDirectoryIterators = new ResourceTracker();
> +     let withDir = function withDir(fd, f, avl) {

You should find a better name than |avl|. Maybe |failIfAbsent|.
Also, please document it.

@@ +156,3 @@
>         let file = OpenedDirectoryIterators.get(fd);
>         if (file == null) {
> +         if (avl == null)  {

|avl| (or |failIfAbsent|) should be a boolean.

@@ +280,5 @@
>                   OpenedDirectoryIterators.remove(dir);
>                 }
>                 throw x;
>               }
> +           }, 1);

Er... 1? That's weird.
Attachment #701020 - Flags: feedback?(dteller) → feedback+
Attached patch patch 0.5 (obsolete) — Splinter Review
Attachment #701020 - Attachment is obsolete: true
Attachment #701032 - Flags: review?(dteller)
Comment on attachment 701032 [details] [diff] [review]
patch 0.5

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +136,5 @@
>        * Execute a function in the context of a given file.
>        *
>        * @param {*} id A unique identifier, as used by |OpenFiles|.
>        * @param {Function} f A function to call.
> +      * @param {boolean} failIfAbsent A variable determining whether to throw an error when the directory doesn't exist 

« If |true|, throw an error when the directory doesn't exist. Otherwise, ignore non-existent directories. »

@@ +157,3 @@
>         let file = OpenedDirectoryIterators.get(fd);
>         if (file == null) {
> +         if (failIfAbsent == true)  { 

Nit: Please remove one whitespace before { and one after.
Also, instead of |if (failIfAbsent == true)|, please write |if (failIfAbsent)|

@@ +301,5 @@
>           return withDir(dir,
>             function do_close() {
>               this.close();
>               OpenedDirectoryIterators.remove(dir);
> +           }, true);

Nope, that's the contrary. This one should be |false| (and please document why), the other ones should be |true|.
Attachment #701032 - Flags: review?(dteller) → feedback+
Attached patch patch 0.8 (obsolete) — Splinter Review
Changes recommended by  David Rajchenbach Teller [:Yoric] applied.
Attachment #701032 - Attachment is obsolete: true
Attached patch patch 0.85 (obsolete) — Splinter Review
Attachment #701076 - Attachment is obsolete: true
Attachment #703174 - Flags: review?(dteller)
Comment on attachment 703174 [details] [diff] [review]
patch 0.85

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

You have my r+ with the minor changes requested.

::: toolkit/components/osfile/osfile_async_worker.js
@@ +142,5 @@
>        * Execute a function in the context of a given file.
>        *
>        * @param {*} id A unique identifier, as used by |OpenFiles|.
>        * @param {Function} f A function to call.
> +      * @param {boolean} failIfAbsent If |true|, throw an error when the directory doesn't exist. Otherwise, ignore non-existent directories.

I just realized that the API would be better with the opposite conventions, i.e. a parameter |ignoreAbsent|. If |true|, the error is ignored, otherwise, the error causes an exception.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +561,5 @@
> +    test.info("Double closing DirectoryIterator");
> +    iterator = new OS.File.DirectoryIterator(currentDir);
> +    yield iterator.close();
> +    yield iterator.close(); //double closing |DirectoryIterator| 
> +    test.ok(true, "|DirectoryIterator| was closed twice successfully");    

That looks good. Could you please remove trailing whitespaces, though?
Attachment #703174 - Flags: review?(dteller) → review+
Attached patch patch-bug 808492Splinter Review
changes applied
Attachment #703174 - Attachment is obsolete: true
Have you run the patch through the TryServer?
Is it ready for checkin-needed?
(don't forget http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed )
Flags: needinfo?(nobody)
Assignee: nobody → miteshpathak05
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ea7f9fd355e9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.