Pier Angelo Vendrame pushed to branch tor-browser-102.9.0esr-12.5-1 at The Tor Project / Applications / Tor Browser

Commits: 6dfea410 by Pier Angelo Vendrame at 2023-04-04T18:32:53+02:00 fixup! Bug 40933: Add tor-launcher functionality

Bug 41709: Reimplement the failure logic for TorProtocolService.sendCommand

sendCommand tries to send a command forever every 250ms in case of failure. That is bad because some async code branch might be stuck in it forever.

We should have a number of maximum attempts and increase the delay after which we try again.

- - - - -

2 changed files:

- toolkit/components/tor-launcher/TorMonitorService.jsm - toolkit/components/tor-launcher/TorProtocolService.jsm

Changes:

===================================== toolkit/components/tor-launcher/TorMonitorService.jsm ===================================== @@ -233,8 +233,8 @@ const TorMonitorService = {

// FIXME: TorProcess is misleading here. We should use a topic related // to having a control port connection, instead. + logger.info(`Notifying ${TorTopics.ProcessIsReady}`); Services.obs.notifyObservers(null, TorTopics.ProcessIsReady); - logger.info(`Notified ${TorTopics.ProcessIsReady}`);

// We reset this here hoping that _shutDownEventMonitor can interrupt // the current monitor, either by calling clearTimeout and preventing it

===================================== toolkit/components/tor-launcher/TorProtocolService.jsm ===================================== @@ -220,49 +220,29 @@ const TorProtocolService = { // Executes a command on the control port. // Return a reply object or null if a fatal error occurs. async sendCommand(cmd, args) { - let conn, reply; - const maxAttempts = 2; - for (let attempt = 0; !reply && attempt < maxAttempts; attempt++) { - try { - conn = await this._getConnection(); - try { - if (conn) { - reply = await conn.sendCommand(cmd + (args ? " " + args : "")); - if (reply) { - // Return for reuse. - this._returnConnection(); - } else { - // Connection is bad. - logger.warn( - "sendCommand returned an empty response, taking the connection as broken and closing it." - ); - this._closeConnection(); - } - } - } catch (e) { - logger.error(`Cannot send the command ${cmd}`, e); - this._closeConnection(); - } - } catch (e) { - logger.error("Cannot get a connection to the control port", e); + const maxTimeout = 1000; + let leftConnAttempts = 5; + let timeout = 250; + let reply; + while (leftConnAttempts-- > 0) { + const response = await this._trySend(cmd, args, leftConnAttempts == 0); + if (response.connected) { + reply = response.reply; + break; } - } - - // We failed to acquire the controller after multiple attempts. - // Try again after some time. - if (!conn) { - logger.info( - "sendCommand: Acquiring control connection failed", + // We failed to acquire the controller after multiple attempts. + // Try again after some time. + logger.warn( + "sendCommand: Acquiring control connection failed, trying again later.", cmd, args ); - return new Promise(resolve => - setTimeout(() => { - resolve(this.sendCommand(cmd, args)); - }, 250) - ); + await new Promise(resolve => setTimeout(() => resolve(), timeout)); + timeout = Math.min(2 * timeout, maxTimeout); }

+ // We sent the command, but we still got an empty response. + // Something must be busted elsewhere. if (!reply) { throw new Error(`${cmd} sent an empty response`); } @@ -590,6 +570,48 @@ const TorProtocolService = { } },

+ async _trySend(cmd, args, rethrow) { + let connected = false; + let reply; + let leftAttempts = 2; + while (leftAttempts-- > 0) { + let conn; + try { + conn = await this._getConnection(); + } catch (e) { + logger.error("Cannot get a connection to the control port", e); + if (leftAttempts == 0 && rethrow) { + throw e; + } + } + if (!conn) { + continue; + } + // If we _ever_ got a connection, the caller should not try again + connected = true; + try { + reply = await conn.sendCommand(cmd + (args ? " " + args : "")); + if (reply) { + // Return for reuse. + this._returnConnection(); + } else { + // Connection is bad. + logger.warn( + "sendCommand returned an empty response, taking the connection as broken and closing it." + ); + this._closeConnection(); + } + } catch (e) { + logger.error(`Cannot send the command ${cmd}`, e); + this._closeConnection(); + if (leftAttempts == 0 && rethrow) { + throw e; + } + } + } + return { connected, reply }; + }, + // Opens an authenticated connection, sets it to this._controlConnection, and // return it. async _getConnection() {

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/commit/6dfea410...