Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling #307

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Improve error handling #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8e14099
Bubble up errors by passing them through.
bradjones1 Mar 17, 2022
2db24ff
Pass through errors that are not HTTP, but include response
bradjones1 Mar 21, 2023
6e31baf
Add response to error object if available on HTTP failures.
bradjones1 Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ export class JSONRPCError extends Error {
public message: string;
public code: number;
public data?: unknown;
// For transports that run over Fetch.
public response?: Response;
constructor(message: string, code: number, data?: any) {
super(message);
this.message = message;
Expand Down
4 changes: 3 additions & 1 deletion src/transports/EventEmitterTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ class EventEmitterTransport extends Transport {
this.transportRequestManager.settlePendingRequest(notifications);
return prom;
} catch (e) {
const responseErr = new JSONRPCError(e.message, ERR_UNKNOWN, e);
const responseErr = e instanceof JSONRPCError
? e
: new JSONRPCError(e.message, ERR_UNKNOWN, e);
this.transportRequestManager.settlePendingRequest(notifications, responseErr);
return Promise.reject(responseErr);
}
Expand Down
12 changes: 9 additions & 3 deletions src/transports/HTTPTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ class HTTPTransport extends Transport {
const notifications = getNotifications(data);
const batch = getBatchRequests(data);
const fetcher = this.injectedFetcher || fetch;
let result;
try {
const result = await fetcher(this.uri, {
result = await fetcher(this.uri, {
method: "POST",
headers: this.headers,
body: JSON.stringify(this.parseData(data)),
Expand All @@ -54,13 +55,18 @@ class HTTPTransport extends Transport {
const body = await result.text();
const responseErr = this.transportRequestManager.resolveResponse(body);
if (responseErr) {
// requirements are that batch requuests are successfully resolved
// requirements are that batch requests are successfully resolved
// this ensures that individual requests within the batch request are settled
this.transportRequestManager.settlePendingRequest(batch, responseErr);
return Promise.reject(responseErr);
}
} catch (e) {
const responseErr = new JSONRPCError(e.message, ERR_UNKNOWN, e);
const responseErr = e instanceof JSONRPCError
? e
: new JSONRPCError(e.message, ERR_UNKNOWN, e);
responseErr.response = result
|| (e?.response instanceof Response && e.response)
|| undefined;
this.transportRequestManager.settlePendingRequest(
notifications,
responseErr
Expand Down
20 changes: 12 additions & 8 deletions src/transports/TransportRequestManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ export class TransportRequestManager {
}
return this.resolveRes(data, emitError);
} catch (e) {
const err = new JSONRPCError("Bad response format", ERR_UNKNOWN, payload);
const err = e instanceof JSONRPCError
? e
: new JSONRPCError("Bad response format", ERR_UNKNOWN, payload);
if (emitError) {
this.transportEventChannel.emit("error", err);
}
Expand Down Expand Up @@ -122,17 +124,19 @@ export class TransportRequestManager {
private resolveRes(data: IJSONRPCNotificationResponse | IJSONRPCResponse, emitError: boolean): TransportResponse {
const { id, error } = data;

const status = this.pendingRequest[id as string];
if (status) {
delete this.pendingRequest[id as string];
this.processResult(data, status);
this.transportEventChannel.emit("response", data as IJSONRPCResponse);
return;
}
if (id === undefined && error === undefined) {
this.transportEventChannel.emit("notification", data as IJSONRPCNotificationResponse);
return;
}
const status = this.pendingRequest[id as string];
if (status) {
delete this.pendingRequest[id as string];
if (error === undefined) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key change. We may have an error response, which per the spec should result in an error response. The prior code block only checked to see if we were expecting the response with a particular ID, and early-returned. The processResult method would still reject the promise, however it would not return an error object.

this.processResult(data, status);
this.transportEventChannel.emit("response", data as IJSONRPCResponse);
return;
}
}
let err;
if (error) {
err = convertJSONToRPCError(data);
Expand Down
5 changes: 3 additions & 2 deletions src/transports/WebSocketTransport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ class WebSocketTransport extends Transport {
this.connection.send(JSON.stringify(this.parseData(data)));
this.transportRequestManager.settlePendingRequest(notifications);
} catch (err) {
const jsonError = new JSONRPCError((err as any).message, ERR_UNKNOWN, err);

const jsonError = err instanceof JSONRPCError
? err
: new JSONRPCError(e.message, ERR_UNKNOWN, err);
this.transportRequestManager.settlePendingRequest(notifications, jsonError);
this.transportRequestManager.settlePendingRequest(getBatchRequests(data), jsonError);

Expand Down