From eb2fc72d580d83eaa6df03bb6df758ddb6a765fa Mon Sep 17 00:00:00 2001 From: Akos Kitta Date: Fri, 17 Mar 2023 15:41:34 +0100 Subject: [PATCH] fix: propagate monitor errors to the frontend Closes #1508 Signed-off-by: Akos Kitta --- .../monitor-manager-proxy-client-impl.ts | 9 +- .../src/browser/monitor-model.ts | 2 +- .../src/common/protocol/monitor-service.ts | 116 +++++++++--- .../src/node/core-service-impl.ts | 4 +- .../src/node/monitor-manager-proxy-impl.ts | 8 +- .../src/node/monitor-manager.ts | 31 ++-- .../src/node/monitor-service.ts | 172 ++++++++---------- i18n/en.json | 5 + 8 files changed, 212 insertions(+), 135 deletions(-) diff --git a/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts b/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts index 94ab4d0f5..b67777edf 100644 --- a/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts +++ b/arduino-ide-extension/src/browser/monitor-manager-proxy-client-impl.ts @@ -1,4 +1,5 @@ import { + ApplicationError, CommandRegistry, Disposable, Emitter, @@ -167,7 +168,13 @@ export class MonitorManagerProxyClientImpl const { selectedBoard, selectedPort } = this.boardsServiceProvider.boardsConfig; if (!selectedBoard || !selectedBoard.fqbn || !selectedPort) return; - await this.server().startMonitor(selectedBoard, selectedPort, settings); + try { + await this.server().startMonitor(selectedBoard, selectedPort, settings); + } catch (err) { + this.messageService.error( + ApplicationError.is(err) ? err.message : String(err) + ); + } } getCurrentSettings(board: Board, port: Port): Promise { diff --git a/arduino-ide-extension/src/browser/monitor-model.ts b/arduino-ide-extension/src/browser/monitor-model.ts index 3ebea1819..1f906378e 100644 --- a/arduino-ide-extension/src/browser/monitor-model.ts +++ b/arduino-ide-extension/src/browser/monitor-model.ts @@ -39,7 +39,7 @@ export class MonitorModel implements FrontendApplicationContribution { this._darkTheme = false; this._wsPort = 0; this._serialPort = ''; - this._connected = true; + this._connected = false; this.onChangeEmitter = new Emitter< MonitorModel.State.Change diff --git a/arduino-ide-extension/src/common/protocol/monitor-service.ts b/arduino-ide-extension/src/common/protocol/monitor-service.ts index 7374951db..541da45ac 100644 --- a/arduino-ide-extension/src/common/protocol/monitor-service.ts +++ b/arduino-ide-extension/src/common/protocol/monitor-service.ts @@ -1,4 +1,4 @@ -import { Event, JsonRpcServer } from '@theia/core'; +import { ApplicationError, Event, JsonRpcServer, nls } from '@theia/core'; import { PluggableMonitorSettings, MonitorSettings, @@ -46,7 +46,7 @@ export interface PluggableMonitorSetting { readonly id: string; // A human-readable label of the setting (to be displayed on the GUI) readonly label: string; - // The setting type (at the moment only "enum" is avaiable) + // The setting type (at the moment only "enum" is available) readonly type: string; // The values allowed on "enum" types readonly values: string[]; @@ -72,24 +72,98 @@ export namespace Monitor { }; } -export interface Status {} -export type OK = Status; -export interface ErrorStatus extends Status { - readonly message: string; +export const MonitorErrorCodes = { + ConnectionFailed: 6001, + NotConnected: 6002, + AlreadyConnected: 6003, + MissingConfiguration: 6004, +} as const; + +export const ConnectionFailedError = createMonitorError( + MonitorErrorCodes.ConnectionFailed +); +export const NotConnectedError = createMonitorError( + MonitorErrorCodes.NotConnected +); +export const AlreadyConnectedError = createMonitorError( + MonitorErrorCodes.AlreadyConnected +); +export const MissingConfigurationError = createMonitorError( + MonitorErrorCodes.MissingConfiguration +); + +export function createConnectionFailedError( + port: Port, + details?: string +): ApplicationError { + const { protocol, protocolLabel, address, addressLabel } = port; + const formattedDetails = details ? `: ${details}.` : '.'; + return ConnectionFailedError( + nls.localize( + 'arduino/monitor/connectionFailedError', + 'Could not connect to {0} {1} port{2}', + addressLabel, + protocolLabel, + formattedDetails + ), + { protocol, address } + ); } -export namespace Status { - export function isOK(status: Status & { message?: string }): status is OK { - return !!status && typeof status.message !== 'string'; - } - export const OK: OK = {}; - export const NOT_CONNECTED: ErrorStatus = { message: 'Not connected.' }; - export const ALREADY_CONNECTED: ErrorStatus = { - message: 'Already connected.', - }; - export const CONFIG_MISSING: ErrorStatus = { - message: 'Serial Config missing.', - }; - export const UPLOAD_IN_PROGRESS: ErrorStatus = { - message: 'Upload in progress.', - }; +export function createNotConnectedError( + port: Port +): ApplicationError { + const { protocol, protocolLabel, address, addressLabel } = port; + return NotConnectedError( + nls.localize( + 'arduino/monitor/notConnectedError', + 'Not connected to {0} {1} port.', + addressLabel, + protocolLabel + ), + { protocol, address } + ); +} +export function createAlreadyConnectedError( + port: Port +): ApplicationError { + const { protocol, protocolLabel, address, addressLabel } = port; + return AlreadyConnectedError( + nls.localize( + 'arduino/monitor/alreadyConnectedError', + 'Could not connect to {0} {1} port. Already connected.', + addressLabel, + protocolLabel + ), + { protocol, address } + ); +} +export function createMissingConfigurationError( + port: Port +): ApplicationError { + const { protocol, protocolLabel, address, addressLabel } = port; + return MissingConfigurationError( + nls.localize( + 'arduino/monitor/missingConfigurationError', + 'Could not connect to {0} {1} port. The monitor configuration is missing.', + addressLabel, + protocolLabel + ), + { protocol, address } + ); +} + +/** + * Bare minimum representation of a port. Supports neither UI labels nor properties. + */ +interface PortDescriptor { + readonly protocol: string; + readonly address: string; +} +function createMonitorError( + code: number +): ApplicationError.Constructor { + return ApplicationError.declare( + code, + (message: string, data: PortDescriptor) => ({ data, message }) + ); } diff --git a/arduino-ide-extension/src/node/core-service-impl.ts b/arduino-ide-extension/src/node/core-service-impl.ts index f438ca2e6..08cfe4260 100644 --- a/arduino-ide-extension/src/node/core-service-impl.ts +++ b/arduino-ide-extension/src/node/core-service-impl.ts @@ -23,7 +23,7 @@ import { UploadUsingProgrammerResponse, } from './cli-protocol/cc/arduino/cli/commands/v1/upload_pb'; import { ResponseService } from '../common/protocol/response-service'; -import { OutputMessage, Port, Status } from '../common/protocol'; +import { OutputMessage, Port } from '../common/protocol'; import { ArduinoCoreServiceClient } from './cli-protocol/cc/arduino/cli/commands/v1/commands_grpc_pb'; import { Port as RpcPort } from './cli-protocol/cc/arduino/cli/commands/v1/port_pb'; import { ApplicationError, CommandService, Disposable, nls } from '@theia/core'; @@ -392,7 +392,7 @@ export class CoreServiceImpl extends CoreClientAware implements CoreService { }: { fqbn?: string | undefined; port?: Port | undefined; - }): Promise { + }): Promise { this.boardDiscovery.setUploadInProgress(false); return this.monitorManager.notifyUploadFinished(fqbn, port); } diff --git a/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts b/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts index 7f284ac3f..95b0a191a 100644 --- a/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts +++ b/arduino-ide-extension/src/node/monitor-manager-proxy-impl.ts @@ -2,7 +2,6 @@ import { inject, injectable } from '@theia/core/shared/inversify'; import { MonitorManagerProxy, MonitorManagerProxyClient, - Status, } from '../common/protocol'; import { Board, Port } from '../common/protocol'; import { MonitorManager } from './monitor-manager'; @@ -41,11 +40,8 @@ export class MonitorManagerProxyImpl implements MonitorManagerProxy { await this.changeMonitorSettings(board, port, settings); } - const connectToClient = (status: Status) => { - if (status === Status.ALREADY_CONNECTED || status === Status.OK) { - // Monitor started correctly, connect it with the frontend - this.client.connect(this.manager.getWebsocketAddressPort(board, port)); - } + const connectToClient = () => { + this.client.connect(this.manager.getWebsocketAddressPort(board, port)); }; return this.manager.startMonitor(board, port, connectToClient); } diff --git a/arduino-ide-extension/src/node/monitor-manager.ts b/arduino-ide-extension/src/node/monitor-manager.ts index 38360a39f..11022e74a 100644 --- a/arduino-ide-extension/src/node/monitor-manager.ts +++ b/arduino-ide-extension/src/node/monitor-manager.ts @@ -1,6 +1,11 @@ import { ILogger } from '@theia/core'; import { inject, injectable, named } from '@theia/core/shared/inversify'; -import { Board, BoardsService, Port, Status } from '../common/protocol'; +import { + AlreadyConnectedError, + Board, + BoardsService, + Port, +} from '../common/protocol'; import { CoreClientAware } from './core-client-provider'; import { MonitorService } from './monitor-service'; import { MonitorServiceFactory } from './monitor-service-factory'; @@ -36,7 +41,7 @@ export class MonitorManager extends CoreClientAware { private monitorServiceStartQueue: { monitorID: string; serviceStartParams: [Board, Port]; - connectToClient: (status: Status) => void; + connectToClient: () => void; }[] = []; @inject(MonitorServiceFactory) @@ -104,7 +109,7 @@ export class MonitorManager extends CoreClientAware { async startMonitor( board: Board, port: Port, - connectToClient: (status: Status) => void + connectToClient: () => void ): Promise { const monitorID = this.monitorID(board.fqbn, port); @@ -127,8 +132,14 @@ export class MonitorManager extends CoreClientAware { return; } - const result = await monitor.start(); - connectToClient(result); + try { + await monitor.start(); + } catch (err) { + if (!AlreadyConnectedError.is(err)) { + throw err; + } + } + await connectToClient(); } /** @@ -202,8 +213,7 @@ export class MonitorManager extends CoreClientAware { async notifyUploadFinished( fqbn?: string | undefined, port?: Port - ): Promise { - let status: Status = Status.NOT_CONNECTED; + ): Promise { let portDidChangeOnUpload = false; // We have no way of knowing which monitor @@ -214,7 +224,7 @@ export class MonitorManager extends CoreClientAware { const monitor = this.monitorServices.get(monitorID); if (monitor) { - status = await monitor.start(); + await monitor.start(); } // this monitorID will only be present in "disposedForUpload" @@ -232,7 +242,6 @@ export class MonitorManager extends CoreClientAware { } await this.startQueuedServices(portDidChangeOnUpload); - return status; } async startQueuedServices(portDidChangeOnUpload: boolean): Promise { @@ -261,8 +270,8 @@ export class MonitorManager extends CoreClientAware { const monitorService = this.monitorServices.get(monitorID); if (monitorService) { - const result = await monitorService.start(); - connectToClient(result); + await monitorService.start(); + await connectToClient(); } } } diff --git a/arduino-ide-extension/src/node/monitor-service.ts b/arduino-ide-extension/src/node/monitor-service.ts index db5f100d7..b3f19ec5f 100644 --- a/arduino-ide-extension/src/node/monitor-service.ts +++ b/arduino-ide-extension/src/node/monitor-service.ts @@ -1,8 +1,22 @@ import { ClientDuplexStream } from '@grpc/grpc-js'; -import { Disposable, Emitter, ILogger } from '@theia/core'; +import { + ApplicationError, + Disposable, + Emitter, + ILogger, + nls, +} from '@theia/core'; import { inject, named, postConstruct } from '@theia/core/shared/inversify'; import { diff, Operation } from 'just-diff'; -import { Board, Port, Status, Monitor } from '../common/protocol'; +import { + Board, + Port, + Monitor, + createAlreadyConnectedError, + createMissingConfigurationError, + createNotConnectedError, + createConnectionFailedError, +} from '../common/protocol'; import { EnumerateMonitorPortSettingsRequest, EnumerateMonitorPortSettingsResponse, @@ -19,8 +33,13 @@ import { PluggableMonitorSettings, MonitorSettingsProvider, } from './monitor-settings/monitor-settings-provider'; -import { Deferred } from '@theia/core/lib/common/promise-util'; +import { + Deferred, + retry, + timeoutReject, +} from '@theia/core/lib/common/promise-util'; import { MonitorServiceFactoryOptions } from './monitor-service-factory'; +import { ServiceError } from './service-error'; export const MonitorServiceName = 'monitor-service'; type DuplexHandlerKeys = @@ -76,7 +95,7 @@ export class MonitorService extends CoreClientAware implements Disposable { readonly onDispose = this.onDisposeEmitter.event; private _initialized = new Deferred(); - private creating: Deferred; + private creating: Deferred; private readonly board: Board; private readonly port: Port; private readonly monitorID: string; @@ -154,25 +173,24 @@ export class MonitorService extends CoreClientAware implements Disposable { /** * Start and connects a monitor using currently set board and port. - * If a monitor is already started or board fqbn, port address and/or protocol - * are missing nothing happens. - * @returns a status to verify connection has been established. + * If a monitor is already started, the promise will reject with an `AlreadyConnectedError`. + * If the board fqbn, port address and/or protocol are missing, the promise rejects with a `MissingConfigurationError`. */ - async start(): Promise { + async start(): Promise { if (this.creating?.state === 'unresolved') return this.creating.promise; this.creating = new Deferred(); if (this.duplex) { this.updateClientsSettings({ monitorUISettings: { connected: true, serialPort: this.port.address }, }); - this.creating.resolve(Status.ALREADY_CONNECTED); + this.creating.reject(createAlreadyConnectedError(this.port)); return this.creating.promise; } if (!this.board?.fqbn || !this.port?.address || !this.port?.protocol) { this.updateClientsSettings({ monitorUISettings: { connected: false } }); - this.creating.resolve(Status.CONFIG_MISSING); + this.creating.reject(createMissingConfigurationError(this.port)); return this.creating.promise; } @@ -218,10 +236,8 @@ export class MonitorService extends CoreClientAware implements Disposable { } monitorRequest.setPortConfiguration(config); - const wroteToStreamSuccessfully = await this.pollWriteToStream( - monitorRequest - ); - if (wroteToStreamSuccessfully) { + try { + await this.pollWriteToStream(monitorRequest); // Only store the config, if the monitor has successfully started. this.currentPortConfigSnapshot = MonitorPortConfiguration.toObject( false, @@ -239,13 +255,20 @@ export class MonitorService extends CoreClientAware implements Disposable { this.updateClientsSettings({ monitorUISettings: { connected: true, serialPort: this.port.address }, }); - this.creating.resolve(Status.OK); + this.creating.resolve(); return this.creating.promise; - } else { + } catch (err) { this.logger.warn( `failed starting monitor to ${this.port?.address} using ${this.port?.protocol}` ); - this.creating.resolve(Status.NOT_CONNECTED); + this.creating.reject( + ApplicationError.is(err) + ? err + : createConnectionFailedError( + this.port, + err instanceof Error ? err.message : String(err) + ) + ); return this.creating.promise; } } @@ -287,21 +310,17 @@ export class MonitorService extends CoreClientAware implements Disposable { } } - pollWriteToStream(request: MonitorRequest): Promise { - let attemptsRemaining = MAX_WRITE_TO_STREAM_TRIES; - const writeTimeoutMs = WRITE_TO_STREAM_TIMEOUT_MS; - + pollWriteToStream(request: MonitorRequest): Promise { const createWriteToStreamExecutor = (duplex: ClientDuplexStream) => - (resolve: (value: boolean) => void, reject: () => void) => { + (resolve: () => void, reject: (reason?: unknown) => void) => { const resolvingDuplexHandlers: DuplexHandler[] = [ { key: 'error', callback: async (err: Error) => { this.logger.error(err); - resolve(false); - // TODO - // this.theiaFEClient?.notifyError() + const details = ServiceError.is(err) ? err.details : err.message; + reject(createConnectionFailedError(this.port, details)); }, }, { @@ -313,79 +332,47 @@ export class MonitorService extends CoreClientAware implements Disposable { return; } if (monitorResponse.getSuccess()) { - resolve(true); + resolve(); return; } const data = monitorResponse.getRxData(); const message = typeof data === 'string' ? data - : this.streamingTextDecoder.decode(data, {stream:true}); + : this.streamingTextDecoder.decode(data, { stream: true }); this.messages.push(...splitLines(message)); }, }, ]; this.setDuplexHandlers(duplex, resolvingDuplexHandlers); - - setTimeout(() => { - reject(); - }, writeTimeoutMs); duplex.write(request); }; - const pollWriteToStream = new Promise((resolve) => { - const startPolling = async () => { - // here we create a new duplex but we don't yet - // set "this.duplex", nor do we use "this.duplex" in our poll - // as duplex 'end' / 'close' events (which we do not "await") - // will set "this.duplex" to null - const createdDuplex = await this.createDuplex(); - - let pollingIsSuccessful; - // attempt a "writeToStream" and "await" CLI response: success (true) or error (false) - // if we get neither within WRITE_TO_STREAM_TIMEOUT_MS or an error we get undefined - try { - const writeToStream = createWriteToStreamExecutor(createdDuplex); - pollingIsSuccessful = await new Promise(writeToStream); - } catch (error) { - this.logger.error(error); - } - - // CLI confirmed port opened successfully - if (pollingIsSuccessful) { - this.duplex = createdDuplex; - resolve(true); - return; - } - - // if "pollingIsSuccessful" is false - // the CLI gave us an error, lets try again - // after waiting 2 seconds if we've not already - // reached MAX_WRITE_TO_STREAM_TRIES - if (pollingIsSuccessful === false) { - attemptsRemaining -= 1; - if (attemptsRemaining > 0) { - setTimeout(startPolling, 2000); - return; - } else { - resolve(false); - return; + return Promise.race([ + retry( + async () => { + let createdDuplex = undefined; + try { + createdDuplex = await this.createDuplex(); + await new Promise(createWriteToStreamExecutor(createdDuplex)); + this.duplex = createdDuplex; + } catch (err) { + createdDuplex?.end(); + throw err; } - } - - // "pollingIsSuccessful" remains undefined: - // we got no response from the CLI within 30 seconds - // resolve to false and end the duplex connection - resolve(false); - createdDuplex.end(); - return; - }; - - startPolling(); - }); - - return pollWriteToStream; + }, + 2_000, + MAX_WRITE_TO_STREAM_TRIES + ), + timeoutReject( + WRITE_TO_STREAM_TIMEOUT_MS, + nls.localize( + 'arduino/monitor/connectionTimeout', + "Timeout. The IDE has not received the 'success' message from the monitor after successfully connecting to it." + ) + ), + ]) as Promise as Promise; } /** @@ -429,9 +416,9 @@ export class MonitorService extends CoreClientAware implements Disposable { * @param message string sent to running monitor * @returns a status to verify message has been sent. */ - async send(message: string): Promise { + async send(message: string): Promise { if (!this.duplex) { - return Status.NOT_CONNECTED; + throw createNotConnectedError(this.port); } const coreClient = await this.coreClient; const { instance } = coreClient; @@ -439,14 +426,12 @@ export class MonitorService extends CoreClientAware implements Disposable { const req = new MonitorRequest(); req.setInstance(instance); req.setTxData(new TextEncoder().encode(message)); - return new Promise((resolve) => { + return new Promise((resolve, reject) => { if (this.duplex) { - this.duplex?.write(req, () => { - resolve(Status.OK); - }); + this.duplex?.write(req, resolve); return; } - this.stop().then(() => resolve(Status.NOT_CONNECTED)); + this.stop().then(() => reject(createNotConnectedError(this.port))); }); } @@ -510,7 +495,7 @@ export class MonitorService extends CoreClientAware implements Disposable { * @param settings map of monitor settings to change * @returns a status to verify settings have been sent. */ - async changeSettings(settings: MonitorSettings): Promise { + async changeSettings(settings: MonitorSettings): Promise { const config = new MonitorPortConfiguration(); const { pluggableMonitorSettings } = settings; const reconciledSettings = await this.monitorSettingsProvider.setSettings( @@ -537,7 +522,9 @@ export class MonitorService extends CoreClientAware implements Disposable { }); if (!this.duplex) { - return Status.NOT_CONNECTED; + // instead of throwing an error, return silently like the original logic + // https://github.com/arduino/arduino-ide/blob/9b49712669b06c97bda68a1e5f04eee4664c13f8/arduino-ide-extension/src/node/monitor-service.ts#L540 + return; } const diffConfig = this.maybeUpdatePortConfigSnapshot(config); @@ -545,7 +532,7 @@ export class MonitorService extends CoreClientAware implements Disposable { this.logger.info( `No port configuration changes have been detected. No need to send configure commands to the running monitor ${this.port.protocol}:${this.port.address}.` ); - return Status.OK; + return; } const coreClient = await this.coreClient; @@ -560,7 +547,6 @@ export class MonitorService extends CoreClientAware implements Disposable { req.setInstance(instance); req.setPortConfiguration(diffConfig); this.duplex.write(req); - return Status.OK; } /** diff --git a/i18n/en.json b/i18n/en.json index 22419c5bf..d96044252 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -328,6 +328,11 @@ "tools": "Tools" }, "monitor": { + "alreadyConnectedError": "Could not connect to {0} {1} port. Already connected.", + "connectionFailedError": "Could not connect to {0} {1} port{2}", + "connectionTimeout": "Timeout. The IDE has not received the 'success' message from the monitor after successfully connecting to it.", + "missingConfigurationError": "Could not connect to {0} {1} port. The monitor configuration is missing.", + "notConnectedError": "Not connected to {0} {1} port.", "unableToCloseWebSocket": "Unable to close websocket", "unableToConnectToWebSocket": "Unable to connect to websocket" },