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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

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

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 1

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

Attachment: DirectoryIterator should fail consistently across platforms

Comment 2

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

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?

Comment 3

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

Attachment: 1. Testing the behavior

Attaching a more robust implementation.

Comment 4

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

Attachment: 2. Asynchronous front-end

Comment 5

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

Attachment: 3. Unix implementation

Comment 6

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

Attachment: 4. Windows implementation

Comment 7

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

Attachment: 5. Constants

Comment 8

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

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 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Nathan Froyd [:froydnj]]

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.

Comment 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

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

Attachment: 6. Testing the behavior v2

Changing the order of patches, plus adding a few tests.

Comment 13

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

Attachment: 3. Unix implementation v2

Comment 14

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

Attachment: 4. Windows implementation v2

Comment 15

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

Attachment: 1. Constants

Comment 16

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

Attachment: 2. Unix implementation v2

Comment 17

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

Attachment: 3. Windows implementation v2

Comment 18

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

Attachment: 4. Asynchronous front-end

Comment 19

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

Attachment: 5. Testing the behavior v2

Reordered patches prior to landing.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c638eba3a46
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4206b887ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/692f9ca21745
https://hg.mozilla.org/integration/mozilla-inbound/rev/65108c196870
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf2f3b45cb38

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7c638eba3a46
https://hg.mozilla.org/mozilla-central/rev/bd4206b887ff
https://hg.mozilla.org/mozilla-central/rev/692f9ca21745
https://hg.mozilla.org/mozilla-central/rev/65108c196870
https://hg.mozilla.org/mozilla-central/rev/cf2f3b45cb38