866571 - [OS.File] Add createUnique method

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[Raymond Lee [:raymondlee]]

See http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.cpp#52 for porting

Comment 1

[Marcos Aruj]

Attachment: WIP patch

Comment 2

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

(In reply to Raymond Lee [:raymondlee] from comment #0)
> See
> http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.
> cpp#52 for porting

Actually, I am starting to wonder: do we really need this method?

Comment 3

[Raymond Lee [:raymondlee]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> (In reply to Raymond Lee [:raymondlee] from comment #0)
> > See
> > http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileCommon.
> > cpp#52 for porting
> 
> Actually, I am starting to wonder: do we really need this method?

But there are many places using it
http://mxr.mozilla.org/mozilla-central/search?string=.CreateUnique&tree=mozilla-central

Do we want to replace them with async solution?

Comment 4

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

Comment on attachment 747292 [details] [diff] [review]
WIP patch

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

Good start.
Next time, if you want me to look at the patch, please mark it as "feedback?".

::: toolkit/components/osfile/osfile_async_worker.js
@@ +275,5 @@
> +       createUnique: function open(path, mode, options) {
> +         let filePath = Type.path.fromMsg(path);
> +         let file = File.createUnique(filePath, mode, options);
> +         return CreatedFiles.add(file, {
> +           // Adding path information to keep track of cerated files

Good idea, but you can just add it to OpenedFiles.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +252,5 @@
>           }
>         }
>         return error_or_file(UnixFile.open(path, flags, omode));
> +     };
> +

Let's agree on the following semantics:
- |path| is the name of the directory;
- if |options.prefix| is specified, it is preprended to the name of the file;
- if |options.extension| is specified, it is appended to the name of the file;
- the file name will look like
  path + "/" + (options.prefix||"" + "-") + somenumber + (options.suffix||"")

Also, you should not accept a |mode|.

@@ +254,5 @@
>         return error_or_file(UnixFile.open(path, flags, omode));
> +     };
> +
> +    File.createUnique = function Unix_createUnique(path, mode, options = noOptions) {
> +      //TODO: Where do I put this constants, can't find Const?

Nah, |Const| is the constants from libc. The constants are fine here.

@@ +257,5 @@
> +    File.createUnique = function Unix_createUnique(path, mode, options = noOptions) {
> +      //TODO: Where do I put this constants, can't find Const?
> +      let kMaxFilenameLength = 255;
> +      let kMaxExtensionLength = 100;
> +      let kMaxSequenceNumberLength = 5;

Why do you prefix all of this with |k|?
Also, you should not assume that the maximal length is 255 or 100.

@@ +262,5 @@
> +
> +      //TODO: move leafname helpers to osfile_unix_back
> +      let rootDir = path.substring(0, path.lastIndexOf('/'));
> +      let leafname = path.substring(path.lastIndexOf('/'));
> +      let rootname = leafname.substring(0, leafname.lastIndexOf('.'));

You should rather use OS.Path.dirname, OS.Path.basename.
Also, forget about the "rootname" and "suffix".

@@ +264,5 @@
> +      let rootDir = path.substring(0, path.lastIndexOf('/'));
> +      let leafname = path.substring(path.lastIndexOf('/'));
> +      let rootname = leafname.substring(0, leafname.lastIndexOf('.'));
> +      let suffix = leafname.substring(leafname.lastIndexOf('.'));
> +      let longname = (path.length + kMaxSequenceNumberLength > kMaxFilenameLength);

You don't seem to be using |longname|.

@@ +273,5 @@
> +
> +      if (File.exists(path)) {
> +        for (let indx = 1; indx < 10000; indx++) {
> +          try {
> +            path = rootDir + rootname + "-" + indx + suffix;

Please do not concatenate paths. Rather, use |OS.Path.join|.
Also, concatenating |indx| pretty much guarantees that you are going to maximize I/O, whereas we want to minimize it.
You should rather concatenate a timestamp + a random number.

@@ +281,5 @@
> +          }
> +        }
> +      }
> +
> +      return File.open(path, mode, options);

Interestingly, this algorithm is not safe, because |path| could be created between the check at line 274 and the check at line 285. So you will need to do something along the lines of:

try {
  return File.open(path, mode);
  // No need to call File.exists, by the way.
} catch (ex if ex instanceof OS.File.Error && ex.becauseExists()) {
  // Now enter the loop.
}

::: toolkit/components/osfile/osfile_win_front.jsm
@@ +311,4 @@
>         return file;
>       };
>  
> +    File.createUnique = function Win_createUnique(path, mode, options = noOptions) {

Chances are that you can use the exact same code for the Unix version and for the Windows version. If so, it should go in osfile_shared_front.jsm.

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +11,5 @@
> +add_task(function test_unique() {
> +    OS.Shared.DEBUG = true;
> +    let currentDir = yield OS.File.getCurrentDirectory();
> +    let path2 = OS.Path.join(currentDir, "test_unique.js");
> +    yield OS.File.createUnique(path2);

You should at least check that the file has been created.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> Also, concatenating |indx| pretty much guarantees that you are going to
> maximize I/O, whereas we want to minimize it.
> You should rather concatenate a timestamp + a random number.

I'm not sure I understand the concern, can you elaborate? Are you suggesting we change the filename-conflict-resolution behavior from an increasing number to some more random suffix? That would lead to sub-optimal results when this method is used for user-exposed files (e.g. saving downloads).

Comment 6

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

My concern is that, if files "foo", "foo-1", "foo-2", ... "foo-n" exist, the algorithm will perform n+2 opening attempts before succeeded, which is hardly optimal.

Now, in our code base, createUnique is used both with user-exposed files and with apparently non-user exposed files (i.e. in SearchService). For the latter, "foo-timestamp-random number" would certainly be quite sufficient. I therefore believe that we should offer an algorithm designed for non-user exposed files. As this is the only usage scenario we have for this function in the immediate future, I believe that we should concentrate on this.

Side-note: For user-exposed files, if instead of "foo-1", "foo-2", ..., we attempted to open "foo - May 11th 2013", there would certainly be much better chances that the algorithm would succeed immediately.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't think we have any evidence that the algorithm in the current implementation has proven to be a problem in practice, and I don't think this method gets used in any really performance-critical situations, so I'm not inclined to change it.

Comment 8

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

It annoys me a little to have a method that looks atomic but, by default, can actually cause an unbounded number of I/O operations. We already have a few functions that loop (typically, |copy|), so this might not be fatal, but it still annoys me a little. I need to think it over a little.

Comment 9

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

Are we ever using createUnique for user-facing files?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> Are we ever using createUnique for user-facing files?

We are currently, yes (add-ons too). So I don't think we can just ignore that use case if we want people to actually stop doing main-thread I/O.

Some examples:
http://hg.mozilla.org/mozilla-central/annotate/eeb89381c7a3/toolkit/webapps/WebappsInstaller.jsm#l491
https://mxr.mozilla.org/addons/search?string=.createUnique%28

However, it appears that many such users already try to make even "nicer" filenames:

http://hg.mozilla.org/mozilla-central/annotate/eeb89381c7a3/toolkit/mozapps/downloads/DownloadPaths.jsm#l46
http://hg.mozilla.org/mozilla-central/annotate/eeb89381c7a3/browser/metro/components/HelperAppDialog.js#l165
http://hg.mozilla.org/mozilla-central/annotate/eeb89381c7a3/toolkit/content/contentAreaUtils.js#l645
http://hg.mozilla.org/mozilla-central/annotate/eeb89381c7a3/mobile/android/components/HelperAppDialog.js#l163

It pains me that this method is forked in so many places, particularly with the "we can't share code" comment which is patently false. It seems to me like having the low-level OS.File API not address the "user-exposed filename" use case seems reasonable, but we should strive to have a shared method somewhere that does (and then migrate all of these users to it).

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #10)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> > Are we ever using createUnique for user-facing files?
> 
> We are currently, yes (add-ons too). So I don't think we can just ignore
> that use case if we want people to actually stop doing main-thread I/O.

Bleh.

> However, it appears that many such users already try to make even "nicer"
> filenames:

I am amused that these places actually hand filenames off to createUnique.

> It pains me that this method is forked in so many places

In discussing this with David earlier today, I said we don't want a "system" version and a "user-friendly" version.  I guess people really do care about what their temporary filenames look like, though.

I'm thinking something like:

function createUniqueWithGenerator(filename, generator)
{
  // May want a limit on this loop.
  for (let attempt in generator(filename)) {
    if (!OS.File.exists(attempt)) {
      return attempt;
    }
  }
}

People can then implement their own thing if they want pretty filenames and OS.File can implement a GUID-esque generator and default to it in OS.File.createUnique (or what have you).

I think in addition to createUnique (instead of?), we should really have an openUnique that returns [fd, path] (and therefore uses open() to check for existence).

Comment 12

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

> I'm thinking something like:
>
> function createUniqueWithGenerator(filename, generator)

Works for me. It's still pretty much having both a "system" and a "user-friendly" version, though.

> I think in addition to createUnique (instead of?), we should really have an openUnique that returns [fd, path] (and therefore uses open() to check for existence).

"Instead of" does it for me.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> > I'm thinking something like:
> >
> > function createUniqueWithGenerator(filename, generator)
> 
> Works for me. It's still pretty much having both a "system" and a
> "user-friendly" version, though.

Would you like it better if I just said "I was wrong"? :)

> > I think in addition to createUnique (instead of?), we should really have an openUnique that returns [fd, path] (and therefore uses open() to check for existence).
> 
> "Instead of" does it for me.

I would like that too, but it's not clear to me that all of the uses of createUnique can be easily transitioned to openUnique.  createUnique should be using open() to check for existence anyway--it's more file I/O (open, close, open for actual use), but easier to transition code to, perhaps?

Comment 14

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

(In reply to Nathan Froyd (:froydnj) from comment #13)
> Would you like it better if I just said "I was wrong"? :)

:)

> > > I think in addition to createUnique (instead of?), we should really have an openUnique that returns [fd, path] (and therefore uses open() to check for existence).
> > 
> > "Instead of" does it for me.
> 
> I would like that too, but it's not clear to me that all of the uses of
> createUnique can be easily transitioned to openUnique.  createUnique should
> be using open() to check for existence anyway--it's more file I/O (open,
> close, open for actual use), but easier to transition code to, perhaps?

What's your use scenario that would call for a function that closes the file? I can't think of any.

Comment 15

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> > I would like that too, but it's not clear to me that all of the uses of
> > createUnique can be easily transitioned to openUnique.  createUnique should
> > be using open() to check for existence anyway--it's more file I/O (open,
> > close, open for actual use), but easier to transition code to, perhaps?
> 
> What's your use scenario that would call for a function that closes the
> file? I can't think of any.

Sorry, I should be more clear here; createUnique looks something like:

  while (true) {
    fd = open filename in exclusive mode
    if we have a valid fd:
      break;
    filename = generate new filename
  }
  close fd
  return filename

People using createUnique, then, are doing at least one extra open/close to get a unique filename, and then another open to actually write data out to the file.  openUnique would eliminate the close and the redundant open.  This is obviously preferable.  But I'm not sure how easy it'd be to rewrite code to follow that scheme.

Comment 16

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

I'd say: let's start by implementing openUnique. If we find a case in which this is not sufficient, there will still be time to implement createUnique, too.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> I'd say: let's start by implementing openUnique. If we find a case in which
> this is not sufficient, there will still be time to implement createUnique,
> too.

WFM!

Comment 18

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

Your turn, Marcos :)

Comment 19

[Marcos Aruj]

Hi guys. 'openUnique' sounds good. I'll work on that method and if I have any questions I'll ask here or IRC. 

Best regards. ;)

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Any updates, Marco?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Sorry, I meant Marcos :)

Comment 22

[Marcos Aruj]

Hi Gavin,

I'm working on it. I'll post a new WIP patch on Monday to get some feedback from you guys. :)

Comment 23

[Marcos Aruj]

Attachment: WIP Patch

WIP patch. HG import is not working I think. Next patch will work on mercurial too.

Comment 24

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

Comment on attachment 760752 [details] [diff] [review]
WIP Patch

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

::: toolkit/components/osfile/osfile_async_worker.js
@@ +271,4 @@
>             path: filePath
>           });
>         },
> +

Lacks documentation. Yes, I know that you know :)

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +446,5 @@
> + * @rejects {OS.Error}
> + */
> +File.openUnique = function open(path, mode, options) {
> +  return Scheduler.post(
> +      "openUnique", [Type.path.toMsg(path), mode, options],

I believe that you are not using |mode|.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +264,5 @@
> +      let suffix = leafName.substring(leafName.lastIndexOf('.'));
> +
> +      try {
> +        return File.open(path, mode);
> +      } catch (ex) {

catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
  ...
}
(you don't want to catch other errors)

@@ +266,5 @@
> +      try {
> +        return File.open(path, mode);
> +      } catch (ex) {
> +        if (ex instanceof OS.File.Error) {
> +          for (let indx = 1; indx < 10000; indx++) {

* After careful negotiation, Nathan and myself agree that, by default, instead of |fileName + "-" + index + suffix|, we would do something along the lines of |fileName + "-" + long hex number + suffix|. You should keep the human-readable naming scheme only if some option |humanReadable| is set to |true|.
* Also, could you make that 10000 customizable by an option.
* Also, please default that value to 20, so as to avoid trashing the disk and emptying the battery in case of problem.

@@ +269,5 @@
> +        if (ex instanceof OS.File.Error) {
> +          for (let indx = 1; indx < 10000; indx++) {
> +            try {
> +              path = OS.Path.join(rootDir, fileName + "-" + indx + suffix);
> +              return File.open(path, mode);

You should probably return |{path: path, file: File.open(path, mode)}|. This will complicate your code in osfile_async_* slightly.

@@ +271,5 @@
> +            try {
> +              path = OS.Path.join(rootDir, fileName + "-" + indx + suffix);
> +              return File.open(path, mode);
> +            } catch(exc) {
> +              //keep trying;

Again, catch only if it's an OS.File.Error and ex.becauseExists.

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +4,5 @@
> +Components.utils.import("resource://gre/modules/Task.jsm");
> +
> +function run_test() {
> +  do_test_pending();
> +	run_next_test();

Nit: indentation. Also, I seem to remember that you don't need do_test_pending() if you use run_next_test and add_task.

@@ +10,5 @@
> +
> +add_task(function test_unique() {
> +  OS.Shared.DEBUG = true;
> +  let currentDir = yield OS.File.getCurrentDirectory();
> +  let path2 = OS.Path.join(currentDir, "dummy_unique_file.txt");

Why |path2|?

@@ +12,5 @@
> +  OS.Shared.DEBUG = true;
> +  let currentDir = yield OS.File.getCurrentDirectory();
> +  let path2 = OS.Path.join(currentDir, "dummy_unique_file.txt");
> +  yield OS.File.openUnique(path2);
> +  // Close previous do_test_pending() call.

You should check that the resulting file:
- exists;
- is empty;
- has been created at most one minute ago (time precision of file systems is notoriously bad).

@@ +13,5 @@
> +  let currentDir = yield OS.File.getCurrentDirectory();
> +  let path2 = OS.Path.join(currentDir, "dummy_unique_file.txt");
> +  yield OS.File.openUnique(path2);
> +  // Close previous do_test_pending() call.
> +  do_test_finished();

(of course, please remove do_test_finished() if you remove do_test_pending())

Comment 25

[Marcos Aruj]

Attachment: Latest patch for bug 866571

Hi David. I've made changes based on your comments. Please let me know what you think. There are some in-code comments as well.

Thanks,

Marcos.

Comment 26

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

Comment on attachment 765776 [details] [diff] [review]
Latest patch for bug 866571

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

It's nicer.
By the way, why in osfile_unix_front.jsm? The implementation doesn't look Unix-specific.

::: toolkit/components/osfile/osfile_async_worker.js
@@ +279,5 @@
> +           // to report leaks when debugging.
> +           path: openedFile.path
> +         });
> +         openedFile.file = resourceId;
> +         return openedFile;

Instead of overwriting |openedFile.file|, please return a new object.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +450,5 @@
> +      "openUnique", [Type.path.toMsg(path), options],
> +      path
> +    ).then(
> +    function onSuccess(msg) {
> +      msg.file = new File(msg.file);

Please don't overwrite fields like that.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +253,5 @@
>         return error_or_file(UnixFile.open(path, flags, omode));
>       };
>  
> +    /**
> +     * Creates and opens a file. By default, it will generate a random HEX number and use it to create a unique new file name.

Creates and open a file *with a unique name*.

@@ +259,5 @@
> +     * @param {string} path The path to the file.
> +     * @param {*=} options Additional options for file opening. This
> +     * implementation interprets the following fields:
> +     *
> +     * - {number} humanReadable If specified, it will create a new filename with a consecutive int in it. If not,

You can get rid of "it will".

@@ +272,5 @@
> +      let mode = {
> +        create : true
> +      };
> +
> +      let rootDir = OS.Path.dirname(path);

It's not really a root dir, is it?

@@ +278,5 @@
> +      let fileName = leafName.substring(0, leafName.lastIndexOf('.'));
> +      let suffix = leafName.substring(leafName.lastIndexOf('.'));
> +
> +      // Set defaults
> +      options.maxReadableNumber = options.maxReadableNumber || 20;

As you suggest, share this between name generation functions.
Also, please don't overwrite |options|.

@@ +289,5 @@
> +          } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +            //keep trying;
> +          }
> +        }
> +      };

You should take both functions humanReadableName and randomHexName outside of openUnique. This will make openUnique easier to read and one never knows if they may be used for something else.

@@ +303,5 @@
> +          } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +            //keep trying in case of an improbable duplicate filename
> +          }
> +        }
> +      };

I'd go for
let nameGen = // one of the functions above
for (let i = 0; i < numberOfAttempts; ++i) {
  try {
    let path = nameGen();
    return {path: path, file: File.open(path, mode)};
  } catch (ex if ex instanceof ...) {
    // keep trying ...
  }
}

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +6,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +
> +// TODO: How to check for empty file contents and creation datetime?

You shouldn't rely on creation datetime, it's neither portable nor reliable.
To check for empty file contents, use OS.File.stat(), for instance.

@@ +24,5 @@
> +  openedFile = yield OS.File.openUnique(path);
> +  // check the unique file was created
> +  exists = yield OS.File.exists(openedFile.path);
> +  //check new file exists
> +  do_check_true(exists);

Also check that the two file names are distinct.

Also, you should check with both name generation functions.

Comment 27

[Marcos Aruj]

Attachment: Bug-866571.patch

Hi David,

I've made the changes you requested, including tests for both name generation routines. Since both were pretty much the same, I removed them as methods and just created the filename directly. Let me know what you think.

Cheers,

Marcos.

Comment 28

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

Comment on attachment 780251 [details] [diff] [review]
Bug-866571.patch

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

The code looks good to me.
I'd rather have a few more tests, though, as indicated below.

Does this pass tests? I am a little surprised, because there is no code in osfile_{unix, win}_front.jsm to call your code from osfile_shared_front.jsm.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +482,5 @@
>    );
>  };
>  
>  /**
> + * Open a file asynchronously and creates it. If it exists, it generates a new name.

Could you copy the documentation from osfile_shared_front.jsm?

@@ +496,5 @@
> +    ).then(
> +    function onSuccess(msg) {
> +      return {
> +        path: msg.path,
> +        file : new File(msg.file)

Nit: please remove the whitespace before ":"

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +132,5 @@
> + * @param {*=} options Additional options for file opening. This
> + * implementation interprets the following fields:
> + *
> + * - {number} humanReadable If specified, create a new filename with a consecutive int in it. If not,
> + *  it will use HEX numbers.

It's a boolean, not a number. Also, it's "If true", not "if specified."

@@ +134,5 @@
> + *
> + * - {number} humanReadable If specified, create a new filename with a consecutive int in it. If not,
> + *  it will use HEX numbers.
> + * - {number} maxReadableNumber If specified, be used to limit the amount of tries after a failed
> + *  file creation. Default is 20.

"Limit the number of attempts to generate a random HEX number. If unspecified, 20."

@@ +136,5 @@
> + *  it will use HEX numbers.
> + * - {number} maxReadableNumber If specified, be used to limit the amount of tries after a failed
> + *  file creation. Default is 20.
> + *
> + * @return {Object} contains A file object{file} and the path{path}.

@return {*} An object with the following fields:
 - {File} file ...
 - {string} path ...

@@ +157,5 @@
> +    return {path: path, file: OS.File.open(path, mode)};
> +  } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +    for (let i = 0; i < maxAttempts; ++i) {
> +      try {
> +        if (options.humanReadable)

Nit: We prefer if (...) { ... }, even if it's just a one-liner. Saves errors when we make it a more-than-one-liner.

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +6,5 @@
> +function run_test() {
> +  run_next_test();
> +}
> +
> +// TODO: How to check for empty file contents and creation datetime?

Use |OS.File.stat| to get the size of the file.
Creation datetime is not cross-platform and not reliable, so you should probably not rely on it.

@@ +12,5 @@
> +  OS.Shared.DEBUG = true;
> +  let currentDir = yield OS.File.getCurrentDirectory();
> +  let path = OS.Path.join(currentDir, "dummy_unique_file.txt");
> +  let exists = yield OS.File.exists(path);
> +  let isEmpty = false;

For the moment, you're not using isEmpty, so please don't let it lie around.

@@ +25,5 @@
> +  do_check_true(fileInfo.size == 0);
> +
> +  //create unique file - HEX
> +  openedFile = yield OS.File.openUnique(path);
> +  exists = yield OS.File.exists(openedFile.path);

To simplify debugging, you might want to print this path.

@@ +26,5 @@
> +
> +  //create unique file - HEX
> +  openedFile = yield OS.File.openUnique(path);
> +  exists = yield OS.File.exists(openedFile.path);
> +  do_check_true(exists);

Nit: please close the file if you're not using it.

@@ +31,5 @@
> +  let fileInfo = yield OS.File.stat(openedFile.path);
> +  do_check_true(fileInfo.size == 0);
> +
> +  //create unique file - Human Readable
> +  openedFile = yield OS.File.openUnique(path, {humanReadable : true});

Here too, could you please print the path and close the file?

@@ +35,5 @@
> +  openedFile = yield OS.File.openUnique(path, {humanReadable : true});
> +  exists = yield OS.File.exists(openedFile.path);
> +  do_check_true(exists);
> +  let fileInfo = yield OS.File.stat(openedFile.path);
> +  do_check_true(fileInfo.size == 0);

Could you now add a few more tests:
- same thing, but if the file at |path| exists;
- run openUnique several times and ensure that each call returns a different file name.

Comment 29

[Marcos Aruj]

Hi David, I actually was calling the method directly using OS.Shared.AbstractFile.openUnique , which is working. But I've added the methods in unix/win and call the method the regular way.

Comment 30

[Marcos Aruj]

Attachment: Final review patch.

I've made the changes you mention. Currently, I'm calling the Shared method directly using OS.Shared.AbstractFile.openUnique, not creating methods on each platform's file. Is it necessary? How should I add them.

Cheers,

Marcos.

Comment 31

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

Comment on attachment 796175 [details] [diff] [review]
Final review patch.

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

There's still some work, but it's looking good.

::: toolkit/components/osfile/modules/osfile_async_front.jsm
@@ +495,5 @@
> + * @param {*=} options Additional options for file opening. This
> + * implementation interprets the following fields:
> + *
> + * - {number} humanReadable If specified, create a new filename with a consecutive int in it. If not,
> + *  it will use HEX numbers.

I would write "if |true|" rather than if specified.
Also, an example will be nicer than "consecutive int".

::: toolkit/components/osfile/modules/osfile_async_worker.js
@@ +281,5 @@
> +         });
> +
> +         return {
> +           path : openedFile.path,
> +           file : resourceId

Nit: "path:" and "file:", without whitespaces.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +133,5 @@
> + * implementation interprets the following fields:
> + *
> + * - {boolean} humanReadable If true, create a new filename with a consecutive int in it. If not,
> + *  it will use HEX numbers.
> + * - {number} maxAttempts Limit the number of attempts to generate a unique name. If unspecified, 20.

(same remarks as in osfile_async_front.jsm)

@@ +137,5 @@
> + * - {number} maxAttempts Limit the number of attempts to generate a unique name. If unspecified, 20.
> + *
> + * @return {*} An object with the following fields:
> +   - {File} file ...
> +   - {string} path ...{Object} contains A file object{file} and the path{path}.

I suspect that you have forgotten to remove some text.

@@ +148,5 @@
> +
> +  let dirName = OS.Path.dirname(path);
> +  let leafName = OS.Path.basename(path);
> +  let fileName = leafName.substring(0, leafName.lastIndexOf('.'));
> +  let suffix = leafName.substring(leafName.lastIndexOf('.'));

Nit: could you reuse the value of |leafName.lastIndexOf('.')|?

@@ +150,5 @@
> +  let leafName = OS.Path.basename(path);
> +  let fileName = leafName.substring(0, leafName.lastIndexOf('.'));
> +  let suffix = leafName.substring(leafName.lastIndexOf('.'));
> +  let uniquePath = "";
> +  let maxAttempts = options.maxAttempts || 20;

It probably won't change much, but let's set this to 99.

@@ +154,5 @@
> +  let maxAttempts = options.maxAttempts || 20;
> +
> +
> +  try {
> +    return {path: path, file: OS.File.open(path, mode)};

Nit: small style inconsistency between the definition of |mode| and the return value. Please pick one.

@@ +158,5 @@
> +    return {path: path, file: OS.File.open(path, mode)};
> +  } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +    for (let i = 0; i < maxAttempts; ++i) {
> +      try {
> +        if (options.humanReadable) {

Nit: please define |let humanReadable = !!options.humanReadable| and use it here.

@@ +159,5 @@
> +  } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +    for (let i = 0; i < maxAttempts; ++i) {
> +      try {
> +        if (options.humanReadable) {
> +          uniquePath = OS.Path.join(dirName, fileName + "-" + i + suffix);

Actually, if we want this to be human-readable, this should be |(i + 1)|.

@@ +161,5 @@
> +      try {
> +        if (options.humanReadable) {
> +          uniquePath = OS.Path.join(dirName, fileName + "-" + i + suffix);
> +        } else {
> +          let hexNumber = Math.floor(Math.random()*16777215).toString(16);

Nit: Please define the magic constants somewhere as constants.

@@ +169,5 @@
> +      } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +        // keep trying ...
> +      }
> +    }
> +  }

You should do something in case of failure. Probably rethrow the result of OS.File.Error.exists with a meaningful error message.

::: toolkit/components/osfile/modules/osfile_unix_front.jsm
@@ +252,5 @@
>         return error_or_file(UnixFile.open(path, flags, omode));
>       };
>  
> +
> +

Nit: Needless whitespace.

@@ +816,5 @@
>       };
>  
>       File.read = exports.OS.Shared.AbstractFile.read;
>       File.writeAtomic = exports.OS.Shared.AbstractFile.writeAtomic;
> +     File.openUnique = exports.OS.Shared.AbstractFile.openUnique;

Please do this also for the Windows version.

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +37,5 @@
> +  let lastPath = openedFile.path;
> +  for (let i=0; i < 10; i++) {
> +    openedFile = yield OS.File.openUnique(path);
> +    yield openedFile.file.close();
> +    do_check_true(lastPath != openedFile.path);

To ensure that it always generates new file names, you should put the file names in a Set and in the end check that the set contains as many file names as expected.

@@ +56,5 @@
> +    openedFile = yield OS.File.openUnique(path, {humanReadable : true});
> +    yield openedFile.file.close();
> +    do_check_true(lastPath != openedFile.path);
> +  }
> +});

For each file, you should also check that it has the right:
- directory;
- prefix;
- extension.

Also, I would like a test to ensure that it works even if the file does not have an extension.

Also, I would like a test to ensure that files that have not been closed are properly reported when you send notification "test.osfile.web-workers-shutdown".

Also, I would like a test that tests the behavior when the function fails at creating a new file (i.e. attempt to create 100 human-readable files in the same directory).

Comment 32

[Marcos Aruj]

Attachment: WIP

Hi Yoric.

Here's a new patch with almost all of your comments included. However, I need some orientation on this particular test:

"ensure that files that have not been closed are properly reported when you send notification "test.osfile.web-workers-shutdown".

If you could point me or guide me on how to test this I would really appreciate it.

Thanks for your help,

Marcos.

Comment 33

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

(In reply to Marcos Aruj from comment #32)
> "ensure that files that have not been closed are properly reported when you
> send notification "test.osfile.web-workers-shutdown".

The easiest would be to chat with yzen, on #developers. He's the one who introduced the reporting mechanism.

Comment 34

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

Comment on attachment 800004 [details] [diff] [review]
WIP

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

Code looks ready, minus a few trivial changes.
Now, I'd like a few more tests.
Oh, and I'm "yoric", not "yorik" :)

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +152,5 @@
> +  let suffix = (lastDotCharacter != -1 ? leafName.substring(lastDotCharacter) : "");
> +  let uniquePath = "";
> +  let maxAttempts = options.maxAttempts || 99;
> +  let humanReadable = !!options.humanReadable;
> +  const HEX_RADIX= 16;

Nit: missing whitespace.

@@ +153,5 @@
> +  let uniquePath = "";
> +  let maxAttempts = options.maxAttempts || 99;
> +  let humanReadable = !!options.humanReadable;
> +  const HEX_RADIX= 16;
> +  const MAX_HEX_NUMBER = 16777215;

Could you add a comment mentioning that we produce hex numbers between 0 and 2^24 - 1?

@@ +176,5 @@
> +        };
> +      } catch (ex if ex instanceof OS.File.Error && ex.becauseExists) {
> +        // keep trying ...
> +        if (i == maxAttempts)
> +          throw OS.File.Error.exists("openUnique");

Could you move that |throw| out of the loop?
Also, |openUnique| is not a very useful message. Maybe "could not find an unused file name."

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +15,5 @@
> +    let exists = yield OS.File.exists(path);
> +    //check it doesn't exist first
> +    do_check_false(exists);
> +
> +    //create file

Could you add some context for your comments?
e.g. "Ensure that openUnique() uses the file name if there is no file with that name already." (or something better, if you have inspiration)
Same thing for the other comments.

@@ +20,5 @@
> +    let openedFile = yield OS.File.openUnique(path);
> +    do_print("\nCreate new file: " + openedFile.path);
> +    yield openedFile.file.close();
> +    exists = yield OS.File.exists(openedFile.path);
> +    do_check_true(exists);

Since the file doesn't exist yet, you should check that openedFile.path == path.

@@ +31,5 @@
> +    yield openedFile.file.close();
> +    exists = yield OS.File.exists(openedFile.path);
> +    do_check_true(exists);
> +    let fileInfo = yield OS.File.stat(openedFile.path);
> +    do_check_true(fileInfo.size == 0);

Could you check that the path "looks" right? i.e. same prefix and suffix as the original name.

@@ +66,5 @@
> +      lastPath = openedFile.path;
> +      filenames.add(lastPath);
> +    }
> +
> +    do_check_eq(filenames.size, MAX_TRIES);

Could you add a test to exhaust the number of attempts and ensure that an exception is thrown?

Comment 35

[Marcos Aruj]

Attachment: WIP Patch, missing a test to verify prefix and suffix are the same when creating a unique name.

Comment 36

[Marcos Aruj]

Attachment: Bug-866571.patch

Hi Yoric,

Can you review this patch. I've rebased this patch with the latest changes made in the repository. I've included a test to check if an exception is thrown when the max attempts limit is reached. I was trying to use do_check_throw but couldn't make it work, so I took a different approach with a try-catch block and verifying the correct error was thrown.

Let me know what you think.

Cheers,

Marcos.

Comment 37

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

Comment on attachment 810922 [details] [diff] [review]
Bug-866571.patch

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

Looks good to me.

::: toolkit/components/osfile/modules/osfile_shared_front.jsm
@@ +168,5 @@
> +      try {
> +        if (humanReadable) {
> +          uniquePath = OS.Path.join(dirName, fileName + "-" + (i+1) + suffix);
> +        } else {
> +          let hexNumber = Math.floor(Math.random()*MAX_HEX_NUMBER).toString(HEX_RADIX);

Nit: whitespace around operators (also for i + 1)

::: toolkit/components/osfile/tests/xpcshell/test_unique.js
@@ +12,5 @@
> +    const MAX_TRIES = 10;
> +    let currentDir = yield OS.File.getCurrentDirectory();
> +    let path = OS.Path.join(currentDir, filename);
> +    let exists = yield OS.File.exists(path);
> +    //check it doesn't exist first

Nit: "it" is not clear.
Nit 2: Whitespace after //.

@@ +42,5 @@
> +      openedFile = yield OS.File.openUnique(path);
> +      yield openedFile.file.close();
> +      do_check_true(lastPath != openedFile.path);
> +      lastPath = openedFile.path;
> +      filenames.add(lastPath);

I'm not sure that lastPath is very useful.

@@ +64,5 @@
> +      openedFile = yield OS.File.openUnique(path, {humanReadable : true});
> +      yield openedFile.file.close();
> +      do_check_true(lastPath != openedFile.path);
> +      lastPath = openedFile.path;
> +      filenames.add(lastPath);

Same here.

Comment 38

[Marcos Aruj]

Attachment: Bug-866571.patch

Latest patch, ready for checkin if approved.

Comment 39

[Raymond Lee [:raymondlee]]

@marcos: checkin-needed keyword should only be entered if the patch gets r+ and passes Try.

Comment 40

[Marcos Aruj]

@Ray, thanks. Can you submitted to Try? I think Gavin mentioned we should submitted right away as it consists of very small changes.

Comment 41

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #40)
> @Ray, thanks. Can you submitted to Try? I think Gavin mentioned we should
> submitted right away as it consists of very small changes.

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=e84a82a35fa8

Comment 42

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

Comment on attachment 813676 [details] [diff] [review]
Bug-866571.patch

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

Looks good to me, thanks.
Note that the files may have changed a little, so don't forget to pull the latest version of the code to ensure that your patch has not bitrotten.

Comment 43

[Marcos Aruj]

Attachment: Bug-866571.patch

Rebased with the latest changes. Please push to TRY again.

Comment 44

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #43)
> Created attachment 815752 [details] [diff] [review]
> Bug-866571.patch
> 
> Rebased with the latest changes. Please push to TRY again.

https://tbpl.mozilla.org/?tree=Try&rev=3ef8c6f99619

Comment 45

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #44)
> (In reply to Marcos Aruj from comment #43)
> > Created attachment 815752 [details] [diff] [review]
> > Bug-866571.patch
> > 
> > Rebased with the latest changes. Please push to TRY again.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=3ef8c6f99619

Wrong URL. Should be this one https://tbpl.mozilla.org/?tree=Try&rev=63e9a8ac4d1f

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ae104231ede7

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae104231ede7

Comment 48

[Marco Castelluccio [:marco]]

Could we do something similar for directories?

Comment 49

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

We could. Do you have a use case for this?

Comment 50

[Marco Castelluccio [:marco]]

(In reply to David Rajchenbach Teller [:Yoric] <on PTO until Tuseday, November 5th> from comment #49)
> We could. Do you have a use case for this?

We need to create a unique temporary directory to install a webapp (in WebappsInstaller.jsm).