Bug 890050 - OS.File.writeAtomic should rename by default. r=froydnj

This commit is contained in:
David Rajchenbach-Teller 2013-07-10 14:57:17 -04:00
parent 7014cbc07c
commit 5c6a3e7028
3 changed files with 113 additions and 184 deletions

View File

@ -646,14 +646,10 @@ File.exists = function exists(path) {
* Limitation: In a few extreme cases (hardware failure during the
* write, user unplugging disk during the write, etc.), data may be
* corrupted. If your data is user-critical (e.g. preferences,
* application data, etc.), you may ish to pass option |flush: true|
* to decrease the vulnerability to such extreme cases. Note, however,
* that activating |flush| is expensive in terms of performance and
* battery usage and is not sufficient to totally eliminate this
* vulnarability.
*
* Important note: In the current implementation, option |tmpPath|
* is required.
* application data, etc.), you may wish to consider adding options
* |tmpPath| and/or |flush| to reduce the likelihood of corruption, as
* detailed below. Note that no combination of options can be
* guaranteed to totally eliminate the risk of corruption.
*
* @param {string} path The path of the file to modify.
* @param {Typed Array | C pointer} buffer A buffer containing the bytes to write.
@ -661,14 +657,20 @@ File.exists = function exists(path) {
* of this function. This object may contain the following fields:
* - {number} bytes The number of bytes to write. If unspecified,
* |buffer.byteLength|. Required if |buffer| is a C pointer.
* - {string} tmpPath The path at which to write the temporary file.
* - {string} tmpPath If |null| or unspecified, write all data directly
* to |path|. If specified, write all data to a temporary file called
* |tmpPath| and, once this write is complete, rename the file to
* replace |path|. Performing this additional operation is a little
* slower but also a little safer.
* - {bool} noOverwrite - If set, this function will fail if a file already
* exists at |path|. The |tmpPath| is not overwritten if |path| exist.
* - {bool} flush - If set to |true|, the function will flush the
* file. This is considerably slower but slightly safer: if
* the system shuts down improperly (typically due to a kernel freeze
* exists at |path|.
* - {bool} flush - If |false| or unspecified, return immediately once the
* write is complete. If |true|, before writing, force the operating system
* to write its internal disk buffers to the disk. This is considerably slower
* (not just for the application but for the whole system) but also safer:
* if the system shuts down improperly (typically due to a kernel freeze
* or a power failure) or if the device is disconnected before the buffer
* is flushed, the file has more changes of not being corrupted.
* is flushed, the file has more chances of not being corrupted.
*
* @return {promise}
* @resolves {number} The number of bytes actually written.

View File

@ -314,14 +314,10 @@ AbstractFile.read = function read(path, bytes) {
* Limitation: In a few extreme cases (hardware failure during the
* write, user unplugging disk during the write, etc.), data may be
* corrupted. If your data is user-critical (e.g. preferences,
* application data, etc.), you may ish to pass option |flush: true|
* to decrease the vulnerability to such extreme cases. Note, however,
* that activating |flush| is expensive in terms of performance and
* battery usage and is not sufficient to totally eliminate this
* vulnarability.
*
* Important note: In the current implementation, option |tmpPath|
* is required.
* application data, etc.), you may wish to consider adding options
* |tmpPath| and/or |flush| to reduce the likelihood of corruption, as
* detailed below. Note that no combination of options can be
* guaranteed to totally eliminate the risk of corruption.
*
* @param {string} path The path of the file to modify.
* @param {Typed Array | C pointer} buffer A buffer containing the bytes to write.
@ -329,14 +325,20 @@ AbstractFile.read = function read(path, bytes) {
* of this function. This object may contain the following fields:
* - {number} bytes The number of bytes to write. If unspecified,
* |buffer.byteLength|. Required if |buffer| is a C pointer.
* - {string} tmpPath The path at which to write the temporary file.
* - {string} tmpPath If |null| or unspecified, write all data directly
* to |path|. If specified, write all data to a temporary file called
* |tmpPath| and, once this write is complete, rename the file to
* replace |path|. Performing this additional operation is a little
* slower but also a little safer.
* - {bool} noOverwrite - If set, this function will fail if a file already
* exists at |path|. The |tmpPath| is not overwritten if |path| exist.
* - {bool} flush - If set to |true|, the function will flush the
* file. This is considerably slower but slightly safer: if
* the system shuts down improperly (typically due to a kernel freeze
* exists at |path|.
* - {bool} flush - If |false| or unspecified, return immediately once the
* write is complete. If |true|, before writing, force the operating system
* to write its internal disk buffers to the disk. This is considerably slower
* (not just for the application but for the whole system) but also safer:
* if the system shuts down improperly (typically due to a kernel freeze
* or a power failure) or if the device is disconnected before the buffer
* is flushed, the file has more changes of not being corrupted.
* is flushed, the file has more chances of not being corrupted.
*
* @return {number} The number of bytes actually written.
*/
@ -358,35 +360,36 @@ AbstractFile.writeAtomic =
buffer = new TextEncoder(encoding).encode(buffer);
}
if (!options.flush) {
let bytesWritten = 0;
if (!options.tmpPath) {
// Just write, without any renaming trick
let dest = OS.File.open(path, {write: true, truncate: true});
try {
return dest.write(buffer, options);
bytesWritten = dest.write(buffer, options);
if (options.flush) {
dest.flush();
}
} finally {
dest.close();
}
return bytesWritten;
}
let tmpPath = options.tmpPath;
if (!tmpPath) {
throw new TypeError("Expected option tmpPath");
}
let tmpFile = OS.File.open(tmpPath, {write: true, truncate: true});
let bytesWritten;
let tmpFile = OS.File.open(options.tmpPath, {write: true, truncate: true});
try {
bytesWritten = tmpFile.write(buffer, options);
tmpFile.flush();
if (options.flush) {
tmpFile.flush();
}
} catch (x) {
OS.File.remove(tmpPath);
OS.File.remove(options.tmpPath);
throw x;
} finally {
tmpFile.close();
}
OS.File.move(tmpPath, path, {noCopy: true});
OS.File.move(options.tmpPath, path, {noCopy: true});
return bytesWritten;
};

View File

@ -349,7 +349,7 @@ let test_read_write = maketest("read_write", function read_write(test) {
});
/**
* Test OS.File.prototype.{writeAtomic}
* Test OS.File.writeAtomic
*/
let test_read_write_all = maketest("read_write_all", function read_write_all(test) {
return Task.spawn(function() {
@ -357,157 +357,81 @@ let test_read_write_all = maketest("read_write_all", function read_write_all(tes
"osfile async test read writeAtomic.tmp");
let tmpPath = pathDest + ".tmp";
// Check that read + writeAtomic performs a correct copy
let currentDir = yield OS.File.getCurrentDirectory();
let pathSource = OS.Path.join(currentDir, EXISTING_FILE);
let contents = yield OS.File.read(pathSource);
test.ok(contents, "Obtained contents");
let options = {tmpPath: tmpPath};
let optionsBackup = {tmpPath: tmpPath};
let bytesWritten = yield OS.File.writeAtomic(pathDest, contents, options);
test.is(contents.byteLength, bytesWritten, "Wrote the correct number of bytes");
let test_with_options = function(options, suffix) {
return Task.spawn(function() {
let optionsBackup = JSON.parse(JSON.stringify(options));
// Check that options are not altered
test.is(Object.keys(options).length, Object.keys(optionsBackup).length,
"The number of options was not changed");
for (let k in options) {
test.is(options[k], optionsBackup[k], "Option was not changed");
}
yield reference_compare_files(pathSource, pathDest, test);
// Check that read + writeAtomic performs a correct copy
let currentDir = yield OS.File.getCurrentDirectory();
let pathSource = OS.Path.join(currentDir, EXISTING_FILE);
let contents = yield OS.File.read(pathSource);
test.ok(contents, "Obtained contents");
let bytesWritten = yield OS.File.writeAtomic(pathDest, contents, options);
test.is(contents.byteLength, bytesWritten, "Wrote the correct number of bytes (" + suffix + ")");
// Check that temporary file was removed
test.info("Compare complete");
test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
// Check that options are not altered
test.is(Object.keys(options).length, Object.keys(optionsBackup).length,
"The number of options was not changed");
for (let k in options) {
test.is(options[k], optionsBackup[k], "Option was not changed (" + suffix + ")");
}
yield reference_compare_files(pathSource, pathDest, test);
// Check that writeAtomic fails if noOverwrite is true and the destination
// file already exists!
let view = new Uint8Array(contents.buffer, 10, 200);
try {
options = {tmpPath: tmpPath, noOverwrite: true};
yield OS.File.writeAtomic(pathDest, view, options);
test.fail("With noOverwrite, writeAtomic should have refused to overwrite file");
} catch (err) {
test.info("With noOverwrite, writeAtomic correctly failed");
test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error");
test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists");
}
yield reference_compare_files(pathSource, pathDest, test);
test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
// Check that temporary file was removed or doesn't exist
test.info("Compare complete");
test.ok(!(new FileUtils.File(tmpPath).exists()), "No temporary file at the end of the run (" + suffix + ")");
// Now write a subset
let START = 10;
let LENGTH = 100;
view = new Uint8Array(contents.buffer, START, LENGTH);
bytesWritten = yield OS.File.writeAtomic(pathDest, view, {tmpPath: tmpPath});
test.is(bytesWritten, LENGTH, "Partial write wrote the correct number of bytes");
let array2 = yield OS.File.read(pathDest);
let view1 = new Uint8Array(contents.buffer, START, LENGTH);
test.is(view1.length, array2.length, "Re-read partial write with the correct number of bytes");
for (let i = 0; i < LENGTH; ++i) {
if (view1[i] != array2[i]) {
test.is(view1[i], array2[i], "Offset " + i + " is correct");
}
test.ok(true, "Compared re-read of partial write");
}
// Check that writeAtomic fails if noOverwrite is true and the destination
// file already exists!
let view = new Uint8Array(contents.buffer, 10, 200);
try {
let opt = JSON.parse(JSON.stringify(options));
opt.noOverwrite = true;
yield OS.File.writeAtomic(pathDest, view, opt);
test.fail("With noOverwrite, writeAtomic should have refused to overwrite file (" + suffix + ")");
} catch (err) {
test.info("With noOverwrite, writeAtomic correctly failed (" + suffix + ")");
test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error (" + suffix + ")");
test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists (" + suffix + ")");
}
yield reference_compare_files(pathSource, pathDest, test);
test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed");
// Check that writeAtomic fails if there is no tmpPath.
// FIXME: Remove this as part of bug 793660
try {
yield OS.File.writeAtomic(pathDest, contents, {flush: true});
test.fail("Without a tmpPath, writeAtomic should have failed");
} catch (err) {
test.ok(true, "Without a tmpPath, writeAtomic has failed as expected");
}
// Now write a subset
let START = 10;
let LENGTH = 100;
view = new Uint8Array(contents.buffer, START, LENGTH);
bytesWritten = yield OS.File.writeAtomic(pathDest, view, options);
test.is(bytesWritten, LENGTH, "Partial write wrote the correct number of bytes (" + suffix + ")");
let array2 = yield OS.File.read(pathDest);
let view1 = new Uint8Array(contents.buffer, START, LENGTH);
test.is(view1.length, array2.length, "Re-read partial write with the correct number of bytes (" + suffix + ")");
let decoder = new TextDecoder();
test.is(decoder.decode(view1), decoder.decode(array2), "Comparing re-read of partial write (" + suffix + ")");
// Write strings, default encoding
let ARBITRARY_STRING = "aeiouyâêîôûçß•";
yield OS.File.writeAtomic(pathDest, ARBITRARY_STRING, {tmpPath: tmpPath});
let array = yield OS.File.read(pathDest);
let IN_STRING = (new TextDecoder()).decode(array);
test.is(ARBITRARY_STRING, IN_STRING, "String write + read with default encoding works");
// Write strings, default encoding
let ARBITRARY_STRING = "aeiouyâêîôûçß•";
yield OS.File.writeAtomic(pathDest, ARBITRARY_STRING, options);
let array = yield OS.File.read(pathDest);
let IN_STRING = decoder.decode(array);
test.is(ARBITRARY_STRING, IN_STRING, "String write + read with default encoding works (" + suffix + ")");
yield OS.File.writeAtomic(pathDest, ARBITRARY_STRING, {tmpPath: tmpPath, encoding: "utf-16"});
array = yield OS.File.read(pathDest);
IN_STRING = (new TextDecoder("utf-16")).decode(array);
test.is(ARBITRARY_STRING, IN_STRING, "String write + read with utf-16 encoding works");
let opt16 = JSON.parse(JSON.stringify(options));
opt16.encoding = "utf-16";
yield OS.File.writeAtomic(pathDest, ARBITRARY_STRING, opt16);
array = yield OS.File.read(pathDest);
IN_STRING = (new TextDecoder("utf-16")).decode(array);
test.is(ARBITRARY_STRING, IN_STRING, "String write + read with utf-16 encoding works (" + suffix + ")");
// Cleanup.
OS.File.remove(pathDest);
// Cleanup.
OS.File.remove(pathDest);
});
};
// Same tests with |flush: false|
// Check that read + writeAtomic performs a correct copy
options = {tmpPath: tmpPath, flush: false};
optionsBackup = {tmpPath: tmpPath, flush: false};
bytesWritten = yield OS.File.writeAtomic(pathDest, contents, options);
test.is(contents.byteLength, bytesWritten, "Wrote the correct number of bytes (without flush)");
// Check that options are not altered
test.is(Object.keys(options).length, Object.keys(optionsBackup).length,
"The number of options was not changed (without flush)");
for (let k in options) {
test.is(options[k], optionsBackup[k], "Option was not changed (without flush)");
}
yield reference_compare_files(pathSource, pathDest, test);
// Check that temporary file was removed
test.info("Compare complete (without flush)");
test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed (without flush)");
// Check that writeAtomic fails if noOverwrite is true and the destination
// file already exists!
view = new Uint8Array(contents.buffer, 10, 200);
try {
options = {tmpPath: tmpPath, noOverwrite: true, flush: false};
yield OS.File.writeAtomic(pathDest, view, options);
test.fail("With noOverwrite, writeAtomic should have refused to overwrite file (without flush)");
} catch (err) {
test.info("With noOverwrite, writeAtomic correctly failed (without flush)");
test.ok(err instanceof OS.File.Error, "writeAtomic correctly failed with a file error (without flush)");
test.ok(err.becauseExists, "writeAtomic file error confirmed that the file already exists (without flush)");
}
yield reference_compare_files(pathSource, pathDest, test);
test.ok(!(new FileUtils.File(tmpPath).exists()), "Temporary file was removed (without flush)");
// Now write a subset
START = 10;
LENGTH = 100;
view = new Uint8Array(contents.buffer, START, LENGTH);
bytesWritten = yield OS.File.writeAtomic(pathDest, view, {tmpPath: tmpPath, flush: false});
test.is(bytesWritten, LENGTH, "Partial write wrote the correct number of bytes (without flush)");
array2 = yield OS.File.read(pathDest);
view1 = new Uint8Array(contents.buffer, START, LENGTH);
test.is(view1.length, array2.length, "Re-read partial write with the correct number of bytes (without flush)");
for (let i = 0; i < LENGTH; ++i) {
if (view1[i] != array2[i]) {
test.is(view1[i], array2[i], "Offset " + i + " is correct (without flush)");
}
test.ok(true, "Compared re-read of partial write (without flush)");
}
// Cleanup.
OS.File.remove(pathDest);
// Check that writeAtomic fails when destination path is undefined
try {
let path = undefined;
let options = {tmpPath: tmpPath};
yield OS.File.writeAtomic(path, contents, options);
test.fail("With file path undefined, writeAtomic should have failed");
} catch (err) {
test.ok(err.message == "TypeError: File path should be a (non-empty) string",
"With file path undefined, writeAtomic correctly failed");
}
// Check that writeAtomic fails when destination path is an empty string
try {
let path = "";
let options = {tmpPath: tmpPath};
yield OS.File.writeAtomic(path, contents, options);
test.fail("With file path an empty string, writeAtomic should have failed");
} catch (err) {
test.ok(err.message == "TypeError: File path should be a (non-empty) string",
"With file path an empty string, writeAtomic correctly failed");
}
yield test_with_options({tmpPath: tmpPath}, "Renaming, not flushing");
yield test_with_options({tmpPath: tmpPath, flush: true}, "Renaming, flushing");
yield test_with_options({}, "Not renaming, not flushing");
yield test_with_options({flush: true}, "Not renaming, flushing");
});
});