From 9f708fd861ac3a7791608c2b1c2feac95e8f58da Mon Sep 17 00:00:00 2001 From: Thom Chiovoloni Date: Fri, 22 Sep 2017 11:57:13 -0400 Subject: [PATCH] Bug 1400467 - Ensure services/common/logmanager.js awaits it's cleanup function r=markh MozReview-Commit-ID: thQph1UUA0 --HG-- extra : rebase_source : 705cdd9d07a1a26b568554e07638c0415789b404 --- services/common/logmanager.js | 19 +++++++---- .../tests/unit/test_errorhandler_filelog.js | 33 +++---------------- 2 files changed, 16 insertions(+), 36 deletions(-) diff --git a/services/common/logmanager.js b/services/common/logmanager.js index d64563cf34cf..11fc90ace573 100644 --- a/services/common/logmanager.js +++ b/services/common/logmanager.js @@ -266,19 +266,18 @@ LogManager.prototype = { // logs are grouped in about:sync-log. let filename = reasonPrefix + "-" + this.logFilePrefix + "-" + Date.now() + ".txt"; await this._fileAppender.flushToFile(this._logFileSubDirectoryEntries, filename, this._log); - // It's not completely clear to markh why we only do log cleanups // for errors, but for now the Sync semantics have been copied... // (one theory is that only cleaning up on error makes it less // likely old error logs would be removed, but that's not true if // there are occasional errors - let's address this later!) if (reason == this.ERROR_LOG_WRITTEN && !this._cleaningUpFileLogs) { - this._log.trace("Scheduling cleanup."); - // Note we don't return/await or otherwise wait on this promise - it - // continues in the background - this.cleanupLogs().catch(err => { + this._log.trace("Running cleanup."); + try { + await this.cleanupLogs(); + } catch (err) { this._log.error("Failed to cleanup logs", err); - }); + } } return reason; } catch (ex) { @@ -321,7 +320,13 @@ LogManager.prototype = { + entry.name, ex); } }); - iterator.close(); + // Wait for this to close if we need to (but it might fail if OS.File has + // shut down) + try { + await iterator.close(); + } catch (e) { + this._log.warn("Failed to close directory iterator", e); + } this._cleaningUpFileLogs = false; this._log.debug("Done deleting files."); // This notification is used only for tests. diff --git a/services/sync/tests/unit/test_errorhandler_filelog.js b/services/sync/tests/unit/test_errorhandler_filelog.js index de6d20fbda55..55a7eef9b82f 100644 --- a/services/sync/tests/unit/test_errorhandler_filelog.js +++ b/services/sync/tests/unit/test_errorhandler_filelog.js @@ -162,13 +162,6 @@ add_test(function test_sync_error_logOnError_true() { const MESSAGE = "this WILL show up"; log.info(MESSAGE); - // We need to wait until the log cleanup started by this test is complete - // or the next test will fail as it is ongoing. - Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() { - Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs); - run_next_test(); - }); - Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() { Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog); @@ -194,6 +187,7 @@ add_test(function test_sync_error_logOnError_true() { } Svc.Prefs.resetBranch(""); + run_next_test(); }); }); @@ -229,13 +223,6 @@ add_test(function test_login_error_logOnError_true() { const MESSAGE = "this WILL show up"; log.info(MESSAGE); - // We need to wait until the log cleanup started by this test is complete - // or the next test will fail as it is ongoing. - Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() { - Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs); - run_next_test(); - }); - Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() { Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog); @@ -261,6 +248,7 @@ add_test(function test_login_error_logOnError_true() { } Svc.Prefs.resetBranch(""); + run_next_test(); }); }); @@ -301,13 +289,6 @@ add_test(function test_newFailed_errorLog() { const MESSAGE = "this WILL show up 2"; log.info(MESSAGE); - // We need to wait until the log cleanup started by this test is complete - // or the next test will fail as it is ongoing. - Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() { - Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs); - run_next_test(); - }); - Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() { Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog); @@ -333,7 +314,7 @@ add_test(function test_newFailed_errorLog() { } Svc.Prefs.resetBranch(""); - + run_next_test(); }); }); // newFailed is nonzero -- should write a log. @@ -352,13 +333,6 @@ add_test(function test_newFailed_errorLog() { add_test(function test_errorLog_dumpAddons() { Svc.Prefs.set("log.appender.file.logOnError", true); - // We need to wait until the log cleanup started by this test is complete - // or the next test will fail as it is ongoing. - Svc.Obs.add("services-tests:common:log-manager:cleanup-logs", function onCleanupLogs() { - Svc.Obs.remove("services-tests:common:log-manager:cleanup-logs", onCleanupLogs); - run_next_test(); - }); - Svc.Obs.add("weave:service:reset-file-log", function onResetFileLog() { Svc.Obs.remove("weave:service:reset-file-log", onResetFileLog); @@ -383,6 +357,7 @@ add_test(function test_errorLog_dumpAddons() { } Svc.Prefs.resetBranch(""); + run_next_test(); }); });