Dbaeumer/frail-salamander-amber (#1311)

* Improve error handling. * Fix tests
microsoft · Sep 12, 2023 · 864c8c5 · 864c8c5
1 parent fb0a3d4
commit 864c8c5
Show file tree

Hide file tree

Showing 4 changed files with 48 additions and 40 deletions.
diff --git a/client/src/common/client.ts b/client/src/common/client.ts
@@ -1636,10 +1636,13 @@ export abstract class BaseLanguageClient implements FeatureClient<Middleware, La
 	private async handleConnectionError(error: Error, message: Message | undefined, count: number | undefined): Promise<void> {
 		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
@@ -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
@@ -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
@@ -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"}');