From a266fca5cdd8880c3a4945492b820bf0f157186a Mon Sep 17 00:00:00 2001 From: David Rajchenbach-Teller Date: Mon, 25 Feb 2013 09:07:16 -0500 Subject: [PATCH] Bug 840436 - OS.File.writeAtomic without flush doesn't rename anymore. r=froydnj --- .../components/osfile/osfile_shared_front.jsm | 45 +++++++++------- .../tests/mochi/main_test_osfile_async.js | 54 +++++++++++++++++++ 2 files changed, 81 insertions(+), 18 deletions(-) diff --git a/toolkit/components/osfile/osfile_shared_front.jsm b/toolkit/components/osfile/osfile_shared_front.jsm index a3f5d70cd9a..b7e01202f2d 100644 --- a/toolkit/components/osfile/osfile_shared_front.jsm +++ b/toolkit/components/osfile/osfile_shared_front.jsm @@ -308,17 +308,15 @@ AbstractFile.read = function read(path, bytes) { }; /** - * Write a file, atomically. + * Write a file in one operation. * - * By opposition to a regular |write|, this operation ensures that, - * until the contents are fully written, the destination file is - * not modified. - * - * By default, files are flushed for additional safety, i.e. to lower - * the risks of losing data in case the device is suddenly removed or - * in case of sudden shutdown. This additional safety is important - * for user-critical data (e.g. preferences, application data, etc.) - * but comes at a performance cost. For non-critical data (e.g. cache, + * By default, this operation ensures that, until the contents are + * fully written, the destination file is not modified. By default, + * files are flushed for additional safety, i.e. to lower the risks of + * losing data in case the device is suddenly removed or in case of + * sudden shutdown. This additional safety is important for + * user-critical data (e.g. preferences, application data, etc.) but + * comes at a performance cost. For non-critical data (e.g. cache, * thumbnails, etc.), you may wish to deactivate flushing by passing * option |flush: false|. * @@ -347,23 +345,34 @@ AbstractFile.writeAtomic = function writeAtomic(path, buffer, options) { options = options || noOptions; - let tmpPath = options.tmpPath; - if (!tmpPath) { - throw new TypeError("Expected option tmpPath"); - } - let noOverwrite = options.noOverwrite; if (noOverwrite && OS.File.exists(path)) { throw OS.File.Error.exists("writeAtomic"); } + if ("flush" in options && !options.flush) { + // Just write, without any renaming trick + let dest; + try { + dest = OS.File.open(path, {write: true, truncate: true}); + return dest.write(buffer, options); + } finally { + dest.close(); + } + } + + + let tmpPath = options.tmpPath; + if (!tmpPath) { + throw new TypeError("Expected option tmpPath"); + } + + let tmpFile = OS.File.open(tmpPath, {write: true, truncate: true}); let bytesWritten; try { bytesWritten = tmpFile.write(buffer, options); - if ("flush" in options && options.flush) { - tmpFile.flush(); - } + tmpFile.flush(); } catch (x) { OS.File.remove(tmpPath); throw x; diff --git a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js index d4386f5d54f..7193c643abd 100644 --- a/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js +++ b/toolkit/components/osfile/tests/mochi/main_test_osfile_async.js @@ -402,6 +402,60 @@ let test_read_write_all = maketest("read_write_all", function read_write_all(tes // 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); + }); });