From b29dbb622387287dd45242340631006f8e828b06 Mon Sep 17 00:00:00 2001 From: Emanuele Cesena Date: Wed, 9 Jun 2021 10:13:11 +0200 Subject: [PATCH] Use softdelete to destroy session to prevent race condition and re-save --- package.json | 2 +- src/app/TypeormStore/TypeormStore.test.ts | 61 +++++++++++++++++++++-- src/app/TypeormStore/TypeormStore.ts | 32 ++++++++---- src/app/TypeormStore/onError.test.ts | 10 ++-- src/domain/Session/ISession.ts | 2 + 5 files changed, 89 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 5241eec..c861adf 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "supertest": "^3.1.0", "test-all-versions": "^4.1.0", "tslint": "^5.8.0", - "typeorm": "^0.1.4", + "typeorm": "^0.2.34", "typescript": "^3.8.3" }, "dependencies": { diff --git a/src/app/TypeormStore/TypeormStore.test.ts b/src/app/TypeormStore/TypeormStore.test.ts index a85f51f..3bb8ae7 100644 --- a/src/app/TypeormStore/TypeormStore.test.ts +++ b/src/app/TypeormStore/TypeormStore.test.ts @@ -10,6 +10,7 @@ import { Column, Connection, createConnection, + DeleteDateColumn, Entity, Index, PrimaryColumn, @@ -128,7 +129,32 @@ test("touches, handling error", async (t) => { await ctx.componentWillUnmount(); - await ctx.request.get("/views").expect(/database.*closed/i); + await ctx.request.get("/views").expect(/database.*not\ established/i); +}); + +test("race condition", async (t) => { + const { request, repository, blockLogout, unblockReq1 } = t.context as Test; + + t.is((await request.post("/views")).body, 1); + t.is((await request.post("/views")).body, 2); + + // hold logout until triggered by req to /race + blockLogout.then(async () => { + await request.delete("/views"); + // let /race continue + unblockReq1(); + }); + // call to /race returns as expected as logout happens in between + // because session.views was 2, race increments and returns 3 + t.is((await request.post("/race")).body, 3); + + // the record in the db should be deleted and should NOT be updated + // i.e. views should still be at 2, not re-saved to 3 + const records = await repository.find({ withDeleted: true }); + t.is(JSON.parse(records[0].json).views, 2); + + // session was deleted, so a new call results in a new session + t.is((await request.post("/views")).body, 1); }); test.afterEach(async (t) => { @@ -146,6 +172,9 @@ class Session implements ISession { @PrimaryColumn("varchar", { length: 255 }) public id = ""; + @DeleteDateColumn() + public destroyedAt?: Date; + @Column("text") public json = ""; } @@ -159,16 +188,29 @@ class Test { public ttl = 2; + public unblockReq1: any = null; + public unblockLogout: any = null; + public blockReq1: any = null; + public blockLogout: any = null; + private connection: Connection | undefined; public async componentDidMount() { this.connection = await createConnection({ database: ":memory:", entities: [Session], + // logging: ["query", "error"], synchronize: true, type: "sqlite", }); + this.blockReq1 = new Promise((resolve, _) => { + this.unblockReq1 = resolve; + }); + this.blockLogout = new Promise((resolve, _) => { + this.unblockLogout = resolve; + }); + this.repository = this.connection.getRepository(Session); this.express.use( @@ -195,15 +237,26 @@ class Test { this.express.get("/views", (req, res) => { const session = nullthrows(req.session); - res.json(session.views || 0); + res.json((session as any).views || 0); }); this.express.post("/views", (req, res) => { const session = nullthrows(req.session); - session.views = (session.views || 0) + 1; + (session as any).views = ((session as any).views || 0) + 1; + + res.json((session as any).views); + }); + + this.express.post("/race", async (req, res) => { + const session = nullthrows(req.session); + + (session as any).views = ((session as any).views || 0) + 1; - res.json(session.views); + this.unblockLogout(); + this.blockReq1.then(() => { + return res.json((session as any).views); + }); }); } diff --git a/src/app/TypeormStore/TypeormStore.ts b/src/app/TypeormStore/TypeormStore.ts index b7a1552..b4a1281 100644 --- a/src/app/TypeormStore/TypeormStore.ts +++ b/src/app/TypeormStore/TypeormStore.ts @@ -41,7 +41,7 @@ export class TypeormStore extends Store { } > = {}, ) { - super(options); + super(options as any); this.cleanupLimit = options.cleanupLimit; if (options.limitSubquery !== undefined) { this.limitSubquery = options.limitSubquery; @@ -130,13 +130,25 @@ export class TypeormStore extends Store { ) : Promise.resolve() ) - .then(() => - this.repository.save({ - expiredAt: Date.now() + ttl * 1000, - id: sid, - json, - }), - ) + // @ts-ignore + .then(async () => { + try { + await this.repository.findOneOrFail({ id: sid }, { withDeleted: true }); + this.repository.update({ + destroyedAt: null, + id: sid, + } as any, { + expiredAt: Date.now() + ttl * 1000, + json, + }); + } catch (_) { + this.repository.insert({ + expiredAt: Date.now() + ttl * 1000, + id: sid, + json, + }); + } + }) .then(() => { this.debug("SET complete"); @@ -144,7 +156,7 @@ export class TypeormStore extends Store { fn(); } }) - .catch((er) => { + .catch((er: any) => { if (fn) { fn(er); } @@ -159,7 +171,7 @@ export class TypeormStore extends Store { public destroy = (sid: string | string[], fn?: (error?: any) => void) => { this.debug('DEL "%s"', sid); - Promise.all((Array.isArray(sid) ? sid : [sid]).map((x) => this.repository.delete({ id: x }))) + Promise.all((Array.isArray(sid) ? sid : [sid]).map((x) => this.repository.softDelete({ id: x }))) .then(() => { if (fn) { fn(); diff --git a/src/app/TypeormStore/onError.test.ts b/src/app/TypeormStore/onError.test.ts index 3c43a2d..db419e6 100644 --- a/src/app/TypeormStore/onError.test.ts +++ b/src/app/TypeormStore/onError.test.ts @@ -10,6 +10,7 @@ import { Column, Connection, createConnection, + DeleteDateColumn, Entity, Index, PrimaryColumn, @@ -27,6 +28,9 @@ class Session implements ISession { @PrimaryColumn("varchar", { length: 255 }) public id = ""; + @DeleteDateColumn() + public destroyedAt?: Date; + @Column("text") public json = ""; } @@ -49,12 +53,12 @@ class Test { public route() { this.express.get("/views", (req, res) => { const session = nullthrows(req.session); - res.json(session.views || 0); + res.json((session as any).views || 0); }); this.express.post("/views", (req, res) => { const session = nullthrows(req.session); - session.views = (session.views || 0) + 1; - res.json(session.views); + (session as any).views = ((session as any).views || 0) + 1; + res.json((session as any).views); }); } } diff --git a/src/domain/Session/ISession.ts b/src/domain/Session/ISession.ts index a13ae58..447e2a8 100644 --- a/src/domain/Session/ISession.ts +++ b/src/domain/Session/ISession.ts @@ -3,5 +3,7 @@ export interface ISession { id: string; + destroyedAt?: Date; + json: string; }