From adc7898f50d7b57664b8f56734ec9730228e31a9 Mon Sep 17 00:00:00 2001 From: Joey Parrish Date: Wed, 20 Apr 2022 16:50:18 -0700 Subject: [PATCH] fix: Fix hung launcher on shutdown of Safari (#38) In some cases (notably macOS Safari under GitHub Actions), a call to forceKill() would be triggered during another call to forceKill(). This could cause the second Promise to go unresolved, leading to a hang. On GitHub, eventually, the workflow would be cancelled. This fixes the nested calls to forceKill by making them both resolve on the same event (the shutdown triggered by the first call). This also adds a timeout for shutting down a WebDriver session. Although this does not appear to be the root cause of the hang we were experiencing in GitHub Actions workflows, it should be safer to have this timeout. If we can't stop a WebDriver session gracefully, we will timeout after 5s and end the launcher anyway. Closes #24 --- index.js | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index 547a435..3008b16 100644 --- a/index.js +++ b/index.js @@ -26,6 +26,12 @@ const {installWebDrivers} = require('webdriver-installer'); const DRIVER_CACHE = path.join(os.homedir(), '.webdriver-installer-cache'); fs.mkdirSync(DRIVER_CACHE, {recursive: true}); +// Delay on startup to allow the WebDriver server to start. +const WEBDRIVER_STARTUP_DELAY_SECONDS = 2; + +// If it takes longer than this to close our WebDriver session, give up. +const CLOSE_WEBDRIVER_SESSION_TIMEOUT_SECONDS = 5; + let driversInstalledPromise = null; // Map nodejs OS names to Selenium platform names. @@ -91,7 +97,7 @@ const LocalWebDriverBase = function(baseBrowserDecorator, args, logger) { }); this.on('start', async (url) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); + await delay(WEBDRIVER_STARTUP_DELAY_SECONDS); this.browser.init(this.spec, (error) => { if (error) { @@ -116,9 +122,18 @@ const LocalWebDriverBase = function(baseBrowserDecorator, args, logger) { await this.stopWebdriver_(); }; + this.forceKillOperation_ = null; + this.forceKill = async () => { + // Don't nest force-kill operations. If forceKill() was already called, + // just return the same Promise again. + if (this.state == 'BEING_FORCE_KILLED') { + return this.forceKillOperation_; + } + this.state = 'BEING_FORCE_KILLED'; - await this.stopWebdriver_(); + this.forceKillOperation_ = this.stopWebdriver_(); + await this.forceKillOperation_; }; const originalStart = this.start; @@ -173,17 +188,25 @@ const LocalWebDriverBase = function(baseBrowserDecorator, args, logger) { this.stopWebdriver_ = async () => { if (this.browser) { - await new Promise(resolve => this.browser.quit(resolve)); + // If it takes too long to close the session, give up and move on. + await Promise.race([ + delay(CLOSE_WEBDRIVER_SESSION_TIMEOUT_SECONDS), + new Promise(resolve => this.browser.quit(resolve)), + ]); } - // Now that the driver connection and browser are closed, emit the signal - // that shuts down the driver executable. + // Now that the driver connection and browser are closed (or have timed + // out), emit the signal that shuts down the driver executable. await this.emitAsync('kill'); this.state = 'FINISHED'; }; } +async function delay(seconds) { + await new Promise((resolve) => setTimeout(resolve, seconds * 1000)); +} + // Generate a subclass of LocalWebDriverBase and return it. function generateSubclass( browserName, launcherName, driverCommand, getDriverArgs,