From 864c8c5db398b8f1d4fe4672edb68f3779a8e8a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dirk=20B=C3=A4umer?= Date: Tue, 12 Sep 2023 23:23:08 +0200 Subject: [PATCH] Dbaeumer/frail-salamander-amber (#1311) * Improve error handling. * Fix tests --- client/src/common/client.ts | 5 +- jsonrpc/src/common/messageBuffer.ts | 2 +- jsonrpc/src/common/messageReader.ts | 77 ++++++++++++++------------ jsonrpc/src/node/test/messages.test.ts | 4 +- 4 files changed, 48 insertions(+), 40 deletions(-) diff --git a/client/src/common/client.ts b/client/src/common/client.ts index 1e4e7d990..fef2065a8 100644 --- a/client/src/common/client.ts +++ b/client/src/common/client.ts @@ -1636,10 +1636,13 @@ export abstract class BaseLanguageClient implements FeatureClient { const handlerResult: ErrorHandlerResult = await this._clientOptions.errorHandler!.error(error, message, count); if (handlerResult.action === ErrorAction.Shutdown) { - this.error(handlerResult.message ?? `Client ${this._name}: connection to server is erroring. Shutting down server.`, undefined, handlerResult.handled === true ? false : 'force'); + this.error(handlerResult.message ?? `Client ${this._name}: connection to server is erroring.\n${error.message}\nShutting down server.`, undefined, handlerResult.handled === true ? false : 'force'); this.stop().catch((error) => { this.error(`Stopping server failed`, error, false); }); + } else { + this.error(handlerResult.message ?? + `Client ${this._name}: connection to server is erroring.\n${error.message}`, undefined, handlerResult.handled === true ? false : 'force'); } } diff --git a/jsonrpc/src/common/messageBuffer.ts b/jsonrpc/src/common/messageBuffer.ts index 0acb8f7ae..4a2cd899a 100644 --- a/jsonrpc/src/common/messageBuffer.ts +++ b/jsonrpc/src/common/messageBuffer.ts @@ -100,7 +100,7 @@ export abstract class AbstractMessageBuffer implements RAL.MessageBuffer { const header = headers[i]; const index: number = header.indexOf(':'); if (index === -1) { - throw new Error('Message header must separate key and value using :'); + throw new Error(`Message header must separate key and value using ':'\n${header}`); } const key = header.substr(0, index); const value = header.substr(index + 1).trim(); diff --git a/jsonrpc/src/common/messageReader.ts b/jsonrpc/src/common/messageReader.ts index 40396cd63..55bf6dc31 100644 --- a/jsonrpc/src/common/messageReader.ts +++ b/jsonrpc/src/common/messageReader.ts @@ -211,46 +211,51 @@ export class ReadableStreamMessageReader extends AbstractMessageReader { } private onData(data: Uint8Array): void { - this.buffer.append(data); - while (true) { - if (this.nextMessageLength === -1) { - const headers = this.buffer.tryReadHeaders(true); - if (!headers) { - return; - } - const contentLength = headers.get('content-length'); - if (!contentLength) { - this.fireError(new Error('Header must provide a Content-Length property.')); - return; + try { + + this.buffer.append(data); + while (true) { + if (this.nextMessageLength === -1) { + const headers = this.buffer.tryReadHeaders(true); + if (!headers) { + return; + } + const contentLength = headers.get('content-length'); + if (!contentLength) { + this.fireError(new Error(`Header must provide a Content-Length property.\n${JSON.stringify(Object.fromEntries(headers))}`)); + return; + } + const length = parseInt(contentLength); + if (isNaN(length)) { + this.fireError(new Error(`Content-Length value must be a number. Got ${contentLength}`)); + return; + } + this.nextMessageLength = length; } - const length = parseInt(contentLength); - if (isNaN(length)) { - this.fireError(new Error('Content-Length value must be a number.')); + const body = this.buffer.tryReadBody(this.nextMessageLength); + if (body === undefined) { + /** We haven't received the full message yet. */ + this.setPartialMessageTimer(); return; } - this.nextMessageLength = length; - } - const body = this.buffer.tryReadBody(this.nextMessageLength); - if (body === undefined) { - /** We haven't received the full message yet. */ - this.setPartialMessageTimer(); - return; + this.clearPartialMessageTimer(); + this.nextMessageLength = -1; + // Make sure that we convert one received message after the + // other. Otherwise it could happen that a decoding of a second + // smaller message finished before the decoding of a first larger + // message and then we would deliver the second message first. + this.readSemaphore.lock(async () => { + const bytes: Uint8Array = this.options.contentDecoder !== undefined + ? await this.options.contentDecoder.decode(body) + : body; + const message = await this.options.contentTypeDecoder.decode(bytes, this.options); + this.callback(message); + }).catch((error) => { + this.fireError(error); + }); } - this.clearPartialMessageTimer(); - this.nextMessageLength = -1; - // Make sure that we convert one received message after the - // other. Otherwise it could happen that a decoding of a second - // smaller message finished before the decoding of a first larger - // message and then we would deliver the second message first. - this.readSemaphore.lock(async () => { - const bytes: Uint8Array = this.options.contentDecoder !== undefined - ? await this.options.contentDecoder.decode(body) - : body; - const message = await this.options.contentTypeDecoder.decode(bytes, this.options); - this.callback(message); - }).catch((error) => { - this.fireError(error); - }); + } catch (error) { + this.fireError(error); } } diff --git a/jsonrpc/src/node/test/messages.test.ts b/jsonrpc/src/node/test/messages.test.ts index ec90f9448..7ace59e78 100644 --- a/jsonrpc/src/node/test/messages.test.ts +++ b/jsonrpc/src/node/test/messages.test.ts @@ -329,7 +329,7 @@ suite('Messages', () => { assert.fail('Should not parse a message without a Content-Length'); }); reader.onError((err) => { - assert.strictEqual(err.message, 'Header must provide a Content-Length property.'); + assert.strictEqual(err.message, 'Header must provide a Content-Length property.\n{"not-content-length":"43"}'); done(); }); readable.push('Not-Content-Length: 43\r\n\r\n{"jsonrpc":"2.0","id":1,"method":"example"}'); @@ -343,7 +343,7 @@ suite('Messages', () => { assert.fail('Should not parse a message without a Content-Length'); }); reader.onError((err) => { - assert.strictEqual(err.message, 'Content-Length value must be a number.'); + assert.strictEqual(err.message, 'Content-Length value must be a number. Got NaN'); done(); }); readable.push('Content-Length: NaN\r\n\r\n{"jsonrpc":"2.0","id":1,"method":"example"}');