943422 - Implement FileUtils.openAtomicFileOutputStream()

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

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

At the moment, FileUtils offers a method openSafeFileOutputStream() which opens a safe file output stream. Bug 928321 introduces a faster variant, the atomic file output stream. We should:
- create a method FileUtils.openAtomicFileOutputStream() that opens an atomic output stream;
- add comments in FileUtils.jsm explaining that openAtomicFileOutputStream() is generally better;
- update the documentation.

Comment 1

[Peiyong Lin[:lpy](UTC+8)]

Hi David, I take this bug. Should I wait until Bug 928321 checkin?

Comment 2

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

(In reply to Peiyong Lin[:lpy] from comment #1)
> Hi David, I take this bug. Should I wait until Bug 928321 checkin?

Yes, please.

Comment 3

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943422-v1.patch

Implement |FileUtils.openAtomicFileOutputStream()|

Comment 4

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

Comment on attachment 8344194 [details] [diff] [review]
bug943422-v1.patch

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

Looks good.
I'm not setting r+ yet, because I'd like a test in toolkit/modules/tests/xpcshell/test_FileUtils.js

Comment 5

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943422-V2.patch

Add openAtomicFileOutputStream in toolkit/modules/tests/xpcshell/test_FileUtils.js

By the way, I saw |closeSafeFileOutputStream| in here http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/FileUtils.jsm#125, should I implement |closeAtomicFileOutputStream|?

Thanks!

Comment 6

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943422-V3.patch

Update again. :)  Add |closeAtomicFileOutputStream|, tests in toolkit/modules/tests/xpcshell

Comment 7

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

Comment on attachment 8346529 [details] [diff] [review]
bug943422-V3.patch

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

::: toolkit/modules/tests/xpcshell/test_FileUtils.js
@@ +128,5 @@
>  
>    run_next_test();
>  });
>  
> +add_test(function test_openAtomicFileOutputStream_defaultFlags() {

A bit too much copy&paste for my taste. Could you factor the code from openSafeFileOutputStream and openAtomicFileOutputStream tests, since they differ only from two function calls?

Comment 8

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943422-V4.patch

Thank you :)
I also do this for |test_openFileOutputStream*|.

Comment 9

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

Comment on attachment 8346622 [details] [diff] [review]
bug943422-V4.patch

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

Looks good to me, with a few minor changes.
Have you run this through Try yet?

::: toolkit/modules/tests/xpcshell/test_FileUtils.js
@@ +90,5 @@
>  
>    run_next_test();
>  });
>  
> +let openFileOutputStream_defaultFlags = function (aArg, aFileName) {

"aArg" is not a very useful name, "aKind" would be nicer.

@@ +97,5 @@
> +  if (aArg == "atomic") {
> +    fos = FileUtils.openAtomicFileOutputStream(file);
> +  } else if (aArg == "safe") {
> +    fos = FileUtils.openSafeFileOutputStream(file);
> +  } else {

Just to be on the safe side, can you ensure that the string is always one of the accepted strings ("atomic", "safe", "")?

@@ +128,2 @@
>  
> +let openFileOutputStream_modeFlags = function(aArg, aFileName) {

Same comments below.

Comment 10

[Peiyong Lin[:lpy](UTC+8)]

Attachment: bug943422-Final.patch

use |do_check_true| to ensure aKind to be one of |("atomic", "safe", "")|

Comment 11

[Peiyong Lin[:lpy](UTC+8)]

I set the checkin flag, will update document after patch land. Thank you Yoric :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/935b34e51fed

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/935b34e51fed