Closed Bug 840887 Opened 8 years ago Closed 8 years ago

[OS.File] DirectoryIterator should fail consistently across platforms if the directory doesn't exist

Categories

(Toolkit :: OS.File, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 9 obsolete files)

767 bytes, patch
Yoric
: review+
Details | Diff | Splinter Review
3.97 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
7.95 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
2.75 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
4.99 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
At the moment, attempting to iterate across a non-existing directory:
- fails during |new DirectoryIterator(...)| under Unix;
- fails during the first iteration under Windows.

This is not very convenient for applications that need to iterate through a directory only if the directory exists. We should probably improve this.
Comment on attachment 713858 [details] [diff] [review]
DirectoryIterator should fail consistently across platforms

I have made the following changes:
- in synchronous mode, DirectoryIterator now consistently throws if the directory doesn't exist;
- in asynchronous mode, a new method DirectoryIterator.prototype.exists lets users determine whether the directory effectively exists, without having to wait for an exception to be thrown during iteration.

Nathan, what do you think of this?
Attachment #713858 - Flags: feedback?(nfroyd)
Attached patch 1. Testing the behavior (obsolete) — Splinter Review
Attaching a more robust implementation.
Attachment #713858 - Attachment is obsolete: true
Attachment #713858 - Flags: feedback?(nfroyd)
Attachment #714571 - Flags: review?(nfroyd)
Attached patch 2. Asynchronous front-end (obsolete) — Splinter Review
Attachment #714572 - Flags: review?(nfroyd)
Attached patch 3. Unix implementation (obsolete) — Splinter Review
Attachment #714573 - Flags: review?(nfroyd)
Attached patch 4. Windows implementation (obsolete) — Splinter Review
Attachment #714574 - Flags: review?(nfroyd)
Attached patch 5. Constants (obsolete) — Splinter Review
Attachment #714575 - Flags: review?(khuey)
After more thought, I have decided that the best behavior is the following:
- creating a DirectoryIterator for a non-existing directory is perfectly legal (makes it simpler to match semantics of synchronous and asynchronous implementations);
- on the other hand, actually iterating through a non-existing directory throws an exception;
- a method |exists| returns a boolean for the same purpose, without having to actually iterate through the directory.
Comment on attachment 714571 [details] [diff] [review]
1. Testing the behavior

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

This looks sane enough, but you really want to put this patch last in the series or it may cause problems for bisecting.
Attachment #714571 - Flags: review?(nfroyd) → review+
Attachment #714572 - Flags: review?(nfroyd) → review+
Comment on attachment 714573 [details] [diff] [review]
3. Unix implementation

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

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +316,5 @@
>    OSError.exists = function exists(operation) {
>      return new OSError(operation, OS.Constants.libc.EEXIST);
>    };
> +
> +  OSError.noSuchFile = function exists(operation) {

|function noSuchFile|, I think.
Attachment #714573 - Flags: review?(nfroyd) → review+
Comment on attachment 714574 [details] [diff] [review]
4. Windows implementation

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

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +338,5 @@
>    OSError.exists = function exists(operation) {
>      return new OSError(operation, exports.OS.Constants.Win.ERROR_FILE_EXISTS);
>    };
> +
> +  OSError.noSuchFile = function exists(operation) {

|function noSuchFile|, I think.
Attachment #714574 - Flags: review?(nfroyd) → review+
Attached patch 6. Testing the behavior v2 (obsolete) — Splinter Review
Changing the order of patches, plus adding a few tests.
Attachment #714571 - Attachment is obsolete: true
Attachment #715925 - Flags: review+
Attached patch 3. Unix implementation v2 (obsolete) — Splinter Review
Attachment #714573 - Attachment is obsolete: true
Attachment #715936 - Flags: review+
Attached patch 4. Windows implementation v2 (obsolete) — Splinter Review
Attachment #714574 - Attachment is obsolete: true
Attachment #715937 - Flags: review+
Attached patch 1. ConstantsSplinter Review
Attachment #714575 - Attachment is obsolete: true
Attachment #718429 - Flags: review+
Attachment #715936 - Attachment is obsolete: true
Attachment #718430 - Flags: review+
Attachment #714572 - Attachment is obsolete: true
Attachment #718439 - Flags: review+
Reordered patches prior to landing.
Attachment #715925 - Attachment is obsolete: true
Attachment #718443 - Flags: review+
See Also: → 1413869
You need to log in before you can comment on or make changes to this bug.