Skip to content

Commit c736744

Browse files
authored
fix: guard ctx.create() against ended context after client disconnect (#109)
* fix: guard ctx.create() against ended context after client disconnect Add abortSignal?.aborted checks in wrapMiddleware and wrapRouteHandler to prevent crashes when middleware or route handlers run after the client disconnects and the original context is already ended. * chore: bump @gravity-ui/nodekit devDependency to ^2.10.1
1 parent 2c3dc1d commit c736744

4 files changed

Lines changed: 87 additions & 5 deletions

File tree

package-lock.json

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
"@commitlint/cli": "^17.7.1",
3131
"@commitlint/config-conventional": "^17.7.0",
3232
"@gravity-ui/eslint-config": "^3.2.0",
33-
"@gravity-ui/nodekit": "^2.7.0",
33+
"@gravity-ui/nodekit": "^2.10.1",
3434
"@gravity-ui/prettier-config": "^1.1.0",
3535
"@gravity-ui/tsconfig": "^1.0.0",
3636
"@types/accept-language-parser": "^1.5.6",

src/router.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ function isAllowedMethod(method: string): method is Lowercase<HttpMethod> | 'mou
2323
function wrapMiddleware(fn: AppMiddleware, i?: number): AppMiddleware {
2424
const result: AppMiddleware = async (req, res, next) => {
2525
const reqCtx = req.ctx;
26+
// Skip creating child context if parent is already ended (e.g. client disconnected).
27+
// Optional chaining for backward compatibility with nodekit < 2.5.0 (no abortSignal).
28+
if (reqCtx.abortSignal?.aborted) {
29+
return next();
30+
}
2631
const ctx = reqCtx.create(`${fn.name || `noname-${i}`} middleware`);
2732

2833
let ended = false;
@@ -54,6 +59,11 @@ function wrapRouteHandler(fn: AppRouteHandler, handlerName?: string) {
5459
const handlerNameLocal = handlerName || fn.name || UNNAMED_CONTROLLER;
5560

5661
const handler: AppMiddleware = async (req, res, next) => {
62+
// Skip creating child context if parent is already ended (e.g. client disconnected).
63+
// Optional chaining for backward compatibility with nodekit < 2.5.0 (no abortSignal).
64+
if (req.originalContext.abortSignal?.aborted) {
65+
return;
66+
}
5767
req.ctx = req.originalContext.create(handlerNameLocal);
5868
if (req.routeInfo.handlerName !== handlerNameLocal) {
5969
if (req.routeInfo.handlerName === UNNAMED_CONTROLLER) {

src/tests/context-lifecycle.test.ts

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,4 +326,76 @@ describe('Context Lifecycle', () => {
326326
expect(middlewareOriginalCtx!.abortSignal.aborted).toBe(true);
327327
});
328328
});
329+
330+
describe('Client Disconnect Handling', () => {
331+
it('should skip next middleware when context is already ended', async () => {
332+
let slowMiddlewareCalled = false;
333+
let nextMiddlewareCalled = false;
334+
335+
const slowMiddleware = async (req: Request, _res: Response, next: NextFunction) => {
336+
slowMiddlewareCalled = true;
337+
// Simulate client disconnect by destroying the socket
338+
req.socket.destroy();
339+
// Wait for the close event handler's setImmediate to end the context
340+
await new Promise<void>((resolve) => {
341+
setImmediate(() => setImmediate(() => setImmediate(resolve)));
342+
});
343+
next();
344+
};
345+
346+
const nextMiddleware = (_req: Request, _res: Response, next: NextFunction) => {
347+
nextMiddlewareCalled = true;
348+
next();
349+
};
350+
351+
const nodekit = new NodeKit();
352+
const app = new ExpressKit(nodekit, {
353+
'GET /test': {
354+
beforeAuth: [slowMiddleware, nextMiddleware],
355+
handler: (_req: Request, res: Response) => {
356+
res.json({ok: true});
357+
},
358+
},
359+
});
360+
361+
await request
362+
.agent(app.express)
363+
.get('/test')
364+
.catch(() => {});
365+
366+
expect(slowMiddlewareCalled).toBe(true);
367+
expect(nextMiddlewareCalled).toBe(false);
368+
});
369+
370+
it('should not throw when route handler runs after client disconnect', async () => {
371+
const disconnectMiddleware = async (
372+
req: Request,
373+
_res: Response,
374+
next: NextFunction,
375+
) => {
376+
// Simulate client disconnect by destroying the socket
377+
req.socket.destroy();
378+
// Wait for the close event handler's setImmediate to end the context
379+
await new Promise<void>((resolve) => {
380+
setImmediate(() => setImmediate(() => setImmediate(resolve)));
381+
});
382+
next();
383+
};
384+
385+
const nodekit = new NodeKit();
386+
const app = new ExpressKit(nodekit, {
387+
'GET /test': {
388+
beforeAuth: [disconnectMiddleware],
389+
handler: (_req: Request, res: Response) => {
390+
res.json({ok: true});
391+
},
392+
},
393+
});
394+
395+
await request
396+
.agent(app.express)
397+
.get('/test')
398+
.catch(() => {});
399+
});
400+
});
329401
});

0 commit comments

Comments
 (0)