diff --git a/README.md b/README.md index 30b804c..a1e559d 100644 --- a/README.md +++ b/README.md @@ -39,11 +39,21 @@ They can let processing continue down the stack with `next()`, or complete the r engine.push(function (req, res, next, end) { if (req.skipCache) return next(); res.result = getResultFromCache(req); - end(); + return end(); }); ``` -By passing a _return handler_ to the `next` function, you can get a peek at the result before it returns. +Middleware functions can be `async`: + +```js +engine.push(async function (req, res, next, end) { + if (req.method !== targetMethod) return next(); + res.result = await processTargetMethodRequest(req); + return end(); +}); +``` + +By passing a _return handler_ to the `next` function, you can get a peek at the response before it is returned to the requester. ```js engine.push(function (req, res, next, end) { @@ -73,69 +83,45 @@ const subengine = new JsonRpcEngine(); engine.push(subengine.asMiddleware()); ``` -### `async` Middleware +### Error Handling -If you require your middleware function to be `async`, use `createAsyncMiddleware`: +Errors should be handled by throwing inside middleware functions. -```js -const { createAsyncMiddleware } = require('@metamask/json-rpc-engine'); - -let engine = new RpcEngine(); -engine.push( - createAsyncMiddleware(async (req, res, next) => { - res.result = 42; - next(); - }), -); -``` - -`async` middleware do not take an `end` callback. -Instead, the request ends if the middleware returns without calling `next()`: +For backwards compatibility, you can also pass an error to the `end` callback, +or set the error on the response object, and then call `end` or `next`. +However, errors must **not** be passed to the `next` callback. -```js -engine.push( - createAsyncMiddleware(async (req, res, next) => { - res.result = 42; - /* The request will end when this returns */ - }), -); -``` +Errors always take precedent over results. +If an error is detected, the response's `result` property will be deleted. -The `next` callback of `async` middleware also don't take return handlers. -Instead, you can `await next()`. -When the execution of the middleware resumes, you can work with the response again. +All of the following examples are equivalent. +It does not matter of the middleware function is synchronous or asynchronous. ```js -engine.push( - createAsyncMiddleware(async (req, res, next) => { - res.result = 42; - await next(); - /* Your return handler logic goes here */ - addToMetrics(res); - }), -); -``` +// Throwing is preferred. +engine.push(function (req, res, next, end) { + throw new Error(); +}); -You can freely mix callback-based and `async` middleware: +// For backwards compatibility, you can also do this: +engine.push(function (req, res, next, end) { + end(new Error()); +}); -```js engine.push(function (req, res, next, end) { - if (!isCached(req)) { - return next((cb) => { - insertIntoCache(res, cb); - }); - } - res.result = getResultFromCache(req); + res.error = new Error(); end(); }); -engine.push( - createAsyncMiddleware(async (req, res, next) => { - res.result = 42; - await next(); - addToMetrics(res); - }), -); +engine.push(function (req, res, next, end) { + res.error = new Error(); + next(); +}); + +// INCORRECT. Do not do this: +engine.push(function (req, res, next, end) { + next(new Error()); +}); ``` ### Teardown @@ -164,41 +150,3 @@ engine.destroy(); // will throw an error. engine.handle(req); ``` - -### Gotchas - -Handle errors via `end(err)`, _NOT_ `next(err)`. - -```js -/* INCORRECT */ -engine.push(function (req, res, next, end) { - next(new Error()); -}); - -/* CORRECT */ -engine.push(function (req, res, next, end) { - end(new Error()); -}); -``` - -However, `next()` will detect errors on the response object, and cause -`end(res.error)` to be called. - -```js -engine.push(function (req, res, next, end) { - res.error = new Error(); - next(); /* This will cause end(res.error) to be called. */ -}); -``` - -## Running tests - -Build the project if not already built: - -```bash -yarn build -``` - -```bash -yarn test -``` diff --git a/src/JsonRpcEngine.test.ts b/src/JsonRpcEngine.test.ts index 4f9682f..dace5bf 100644 --- a/src/JsonRpcEngine.test.ts +++ b/src/JsonRpcEngine.test.ts @@ -534,8 +534,10 @@ describe('JsonRpcEngine', () => { }); }); - engine.push(function (_request, _response, next, _end) { + // Async middleware function + engine.push(async function (_request, _response, next, _end) { events.push('2-next'); + await delay(); next(function (callback) { events.push('2-return'); callback(); @@ -590,6 +592,33 @@ describe('JsonRpcEngine', () => { }); }); + it('calls back next handler even if async middleware rejects', async () => { + const engine = new JsonRpcEngine(); + + let sawNextReturnHandlerCalled = false; + + engine.push(function (_req, _res, next, _end) { + next(function (callback) { + sawNextReturnHandlerCalled = true; + callback(); + }); + }); + + engine.push(async function (_req, _res, _next, _end) { + throw new Error('boom'); + }); + + const payload = { id: 1, jsonrpc, method: 'hello' }; + + await new Promise((resolve) => { + engine.handle(payload, (error, _response) => { + expect(error).toBeDefined(); + expect(sawNextReturnHandlerCalled).toBe(true); + resolve(); + }); + }); + }); + it('handles error in next handler', async () => { const engine = new JsonRpcEngine(); @@ -708,3 +737,14 @@ describe('JsonRpcEngine', () => { }); }); }); + +/** + * Delay for a number of milliseconds. + * + * @param ms - The number of milliseconds to delay. + */ +async function delay(ms = 1) { + return new Promise((resolve) => { + setTimeout(() => resolve(), ms); + }); +} diff --git a/src/JsonRpcEngine.ts b/src/JsonRpcEngine.ts index 170d76d..5ef13ed 100644 --- a/src/JsonRpcEngine.ts +++ b/src/JsonRpcEngine.ts @@ -34,7 +34,7 @@ export type JsonRpcMiddleware< res: PendingJsonRpcResponse, next: JsonRpcEngineNextCallback, end: JsonRpcEngineEndCallback, - ): void; + ): void | Promise; destroy?: () => void | Promise; }; @@ -534,49 +534,55 @@ export class JsonRpcEngine extends SafeEventEmitter { middleware: JsonRpcMiddleware, returnHandlers: JsonRpcEngineReturnHandler[], ): Promise<[unknown, boolean]> { - return new Promise((resolve) => { - const end: JsonRpcEngineEndCallback = (error?: unknown) => { - const parsedError = error || response.error; - if (parsedError) { - response.error = serializeError(parsedError); - } - // True indicates that the request should end - resolve([parsedError, true]); - }; - - const next: JsonRpcEngineNextCallback = ( - returnHandler?: JsonRpcEngineReturnHandler, - ) => { - if (response.error) { - end(response.error); - } else { - if (returnHandler) { - if (typeof returnHandler !== 'function') { - end( - new JsonRpcError( - errorCodes.rpc.internal, - `JsonRpcEngine: "next" return handlers must be functions. ` + - `Received "${typeof returnHandler}" for request:\n${jsonify( - request, - )}`, - { request: request as Json }, - ), - ); - } - returnHandlers.push(returnHandler); - } + let resolve: (value: [unknown, boolean]) => void; + const middlewareCallbackPromise = new Promise<[unknown, boolean]>( + (_resolve) => { + resolve = _resolve; + }, + ); - // False indicates that the request should not end - resolve([null, false]); - } - }; + const end: JsonRpcEngineEndCallback = (error?: unknown) => { + const parsedError = error || response.error; + if (parsedError) { + response.error = serializeError(parsedError); + } + // True indicates that the request should end + resolve([parsedError, true]); + }; - try { - middleware(request, response, next, end); - } catch (error: any) { - end(error); + const next: JsonRpcEngineNextCallback = ( + returnHandler?: JsonRpcEngineReturnHandler, + ) => { + if (response.error) { + return end(response.error); } - }); + + if (returnHandler) { + if (typeof returnHandler !== 'function') { + return end( + new JsonRpcError( + errorCodes.rpc.internal, + `JsonRpcEngine: "next" return handlers must be functions. ` + + `Received "${typeof returnHandler}" for request:\n${jsonify( + request, + )}`, + { request: request as Json }, + ), + ); + } + returnHandlers.push(returnHandler); + } + + // False indicates that the request should not end + return resolve([null, false]); + }; + + try { + await middleware(request, response, next, end); + } catch (error: any) { + end(error); + } + return middlewareCallbackPromise; } /** diff --git a/src/createAsyncMiddleware.ts b/src/createAsyncMiddleware.ts index ebf1852..3f8b12a 100644 --- a/src/createAsyncMiddleware.ts +++ b/src/createAsyncMiddleware.ts @@ -21,22 +21,24 @@ export type AsyncJsonrpcMiddleware< type ReturnHandlerCallback = (error: null | Error) => void; /** - * JsonRpcEngine only accepts callback-based middleware directly. - * createAsyncMiddleware exists to enable consumers to pass in async middleware - * functions. + * Originally, JsonRpcEngine could only accept synchronous middleware functions. + * `createAsyncMiddleware` was created to enable consumers to pass in async + * middleware functions. * - * Async middleware have no "end" function. Instead, they "end" if they return - * without calling "next". Rather than passing in explicit return handlers, - * async middleware can simply await "next", and perform operations on the - * response object when execution resumes. + * These async middleware have no `end` function. Instead, they `end` if they + * return without calling `next`. Rather than passing in explicit return + * handlers, async middleware can simply await `next`, and perform operations + * on the response object when execution resumes. * - * To accomplish this, createAsyncMiddleware passes the async middleware a - * wrapped "next" function. That function calls the internal JsonRpcEngine - * "next" function with a return handler that resolves a promise when called. + * To accomplish this, `createAsyncMiddleware` passes the async middleware a + * wrapped `next` function. That function calls the internal `JsonRpcEngine` + * `next` function with a return handler that resolves a promise when called. * * The return handler will always be called. Its resolution of the promise * enables the control flow described above. * + * @deprecated As of version 7.1.0, middleware functions can be asnync. This + * should no longer be used. * @param asyncMiddleware - The asynchronous middleware function to wrap. * @returns The wrapped asynchronous middleware function, ready to be consumed * by JsonRpcEngine. @@ -48,7 +50,7 @@ export function createAsyncMiddleware< asyncMiddleware: AsyncJsonrpcMiddleware, ): JsonRpcMiddleware { // eslint-disable-next-line @typescript-eslint/no-misused-promises - return async (request, response, next, end) => { + return (request, response, next, end) => { // nextPromise is the key to the implementation // it is resolved by the return handler passed to the // "next" function @@ -59,36 +61,58 @@ export function createAsyncMiddleware< let returnHandlerCallback: unknown = null; let nextWasCalled = false; + let endWasCalled = false; // This will be called by the consumer's async middleware. - const asyncNext = async () => { + const innerNext = async () => { nextWasCalled = true; // We pass a return handler to next(). When it is called by the engine, // the consumer's async middleware will resume executing. next((runReturnHandlersCallback) => { - // This callback comes from JsonRpcEngine._runReturnHandlers + // This callback comes from JsonRpcEngine.#runReturnHandlers returnHandlerCallback = runReturnHandlersCallback; resolveNextPromise(); }); await nextPromise; }; - try { - await asyncMiddleware(request, response, asyncNext); - - if (nextWasCalled) { - await nextPromise; // we must wait until the return handler is called - (returnHandlerCallback as ReturnHandlerCallback)(null); - } else { - end(null); - } - } catch (error: any) { - if (returnHandlerCallback) { - (returnHandlerCallback as ReturnHandlerCallback)(error); - } else { + // The control flow gets pretty messy in here, and we have to guard against + // accidentally ending the request multiple times. + // If this function weren't deprecated we probably wouldn't tolerate this. + const innerEnd = (error: null | Error) => { + if (!endWasCalled) { end(error); } - } + endWasCalled = true; + }; + + // We have to await the async middleware in order to process the return handler + // and allow the engine to complete request handling. + (async () => { + try { + await asyncMiddleware(request, response, innerNext); + + if (nextWasCalled) { + await nextPromise; // we must wait until the return handler is called + (returnHandlerCallback as ReturnHandlerCallback)(null); + } else { + innerEnd(null); + } + } catch (error: any) { + if (returnHandlerCallback) { + (returnHandlerCallback as ReturnHandlerCallback)(error); + } else { + innerEnd(error); + } + } + })() + // This is mostly to make the linter happy. It should not be possible to + // hit this in practice. + .catch( + /* istanbul ignore next */ (error) => { + innerEnd(error); + }, + ); }; } diff --git a/src/createScaffoldMiddleware.ts b/src/createScaffoldMiddleware.ts index 05a444a..151bf80 100644 --- a/src/createScaffoldMiddleware.ts +++ b/src/createScaffoldMiddleware.ts @@ -19,7 +19,7 @@ type ScaffoldMiddlewareHandler< export function createScaffoldMiddleware(handlers: { [methodName: string]: ScaffoldMiddlewareHandler; }): JsonRpcMiddleware { - return (req, res, next, end) => { + return async (req, res, next, end) => { const handler = handlers[req.method]; // if no handler, return if (handler === undefined) {