OpenNeo/impress-2020
OpenNeo/impress-2020
1
0
Fork 0
Code Pull requests Activity

Fix db transactions with pooling

Browse source 
Ah okay, pools support `query` and `execute` the same way connection objects do (as a shorthand for acquiring, querying, and releasing), but it doesn't have the same helpers for transactions. Makes sense: you need those queries to go to the same connection, and an API where you just call it against the pool object can't tell that it's part of the same thing!

Now, we have our transaction code explicitly acquire a connection to use for the duration of the transaction.

An alternative considered would have been to have `connectToDb` acquire a connection, and then release it at the end of the GraphQL request. That would have made app code simpler, but added a lot of additional potential surprise failure points to the infra imo (e.g. what if we're misunderstanding the GraphQL codepath and the connection never gets released? whereas here it's relatively easy to audit that there's a `finally` in the right spot.)
This commit is contained in:
Emi Matchu 2022-01-07 18:06:46 -08:00
parent 4ed6344b3d
commit 3744a476e5
3 changed files with 34 additions and 25 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

29
src/server/types/Item.js
View file

@ -1035,11 +1035,12 @@ const resolvers = {
const now = new Date();
await db.beginTransaction();
const connection = await db.getConnection();
try {
await connection.beginTransaction();
if (removeFromDefaultList) {
// First, remove from the default list, if requested.
await db.query(
await connection.query(
`
DELETE FROM closet_hangers
WHERE item_id = ? AND user_id = ? AND list_id IS NULL
@ -1051,7 +1052,7 @@ const resolvers = {
}
// Then, add to the new list.
await db.query(
await connection.query(
`
INSERT INTO closet_hangers
(item_id, user_id, owned, list_id, quantity, created_at, updated_at)
@ -1060,15 +1061,17 @@ const resolvers = {
[itemId, userId, ownsOrWantsItems === "OWNS", listId, 1, now, now]
);
await db.commit();
await connection.commit();
} catch (error) {
try {
await db.rollback();
await connection.rollback();
} catch (error2) {
console.warn(`Error rolling back transaction`, error2);
}
throw error;
} finally {
await connection.release();
}
return closetListRef;
@ -1099,10 +1102,10 @@ const resolvers = {
? [userId, ownsOrWantsItems === "OWNS"]
: [listId];
await db.beginTransaction();
const connection = await db.getConnection();
try {
await db.query(
await connection.beginTransaction();
await connection.query(
`
DELETE FROM closet_hangers
WHERE ${listMatcherCondition} AND item_id = ? LIMIT 1;
@ -1113,7 +1116,7 @@ const resolvers = {
if (ensureInSomeList) {
// If requested, we check whether the item is still in *some* list of
// the same own/want type. If not, we add it to the default list.
const [rows] = await db.query(
const [rows] = await connection.query(
`
SELECT COUNT(*) AS count FROM closet_hangers
WHERE user_id = ? AND item_id = ? AND owned = ?
@ -1123,7 +1126,7 @@ const resolvers = {
if (rows[0].count === 0) {
const now = new Date();
await db.query(
await connection.query(
`
INSERT INTO closet_hangers
(item_id, user_id, owned, list_id, quantity, created_at, updated_at)
@ -1134,13 +1137,15 @@ const resolvers = {
}
}
await db.commit();
await connection.commit();
} catch (error) {
try {
await db.rollback();
await connection.rollback();
} catch (error) {
console.warn(`Error rolling back transaction`, error);
}
} finally {
await connection.release();
}
return closetListRef;

13
src/server/types/MutationsForSupport.js
View file

@ -872,11 +872,11 @@ const resolvers = {
return null;
}
await db.beginTransaction();
const connection = await db.getConnection();
let auth0Warning = null;
try {
const [[result1, result2]] = await db.query(
await connection.beginTransaction();
const [[result1, result2]] = await connection.query(
`
UPDATE users SET name = ? WHERE id = ? LIMIT 1;
UPDATE openneo_id.users SET name = ? WHERE id = ? LIMIT 1;
@ -921,17 +921,18 @@ const resolvers = {
throw error;
}
}
await connection.commit();
} catch (error) {
// If any part of this fails (including the commit to Auth0), roll back
// the database change. This doesn't *super* matter exactly (it's
// probably not *bad* to change the username in the database), but it
// does make the results more obvious and consistent for Support staff.
await db.rollback();
await connection.rollback();
throw error;
} finally {
await connection.release();
}
await db.commit();
if (process.env["SUPPORT_TOOLS_DISCORD_WEBHOOK_URL"]) {
try {
const auth0WarningFields = auth0Warning

17
src/server/types/Outfit.js
View file

@ -208,13 +208,14 @@ const resolvers = {
// Save the outfit, and its item_outfit_relationships rows, in a
// transaction.
await db.beginTransaction();
const connection = await db.getConnection();
let newOutfitId;
try {
await connection.beginTransaction();
// If this is a new outfit, INSERT it. Or, if it's an existing outfit,
// UPDATE it.
const [result] = id
? await db.execute(
? await connection.execute(
`
UPDATE outfits
SET name = ?, pet_state_id = ?,
@ -223,7 +224,7 @@ const resolvers = {
`,
[name, petState.id, id]
)
: await db.execute(
: await connection.execute(
`
INSERT INTO outfits
(name, pet_state_id, user_id, created_at, updated_at)
@ -241,7 +242,7 @@ const resolvers = {
// performing the actual needed sync could be better. Keep an eye
// on query perf!
if (id) {
await db.execute(
await connection.execute(
`DELETE FROM item_outfit_relationships WHERE outfit_id = ?;`,
[id]
);
@ -257,7 +258,7 @@ const resolvers = {
...wornItemIds.map((itemId) => [newOutfitId, itemId, true]),
...closetedItemIds.map((itemId) => [newOutfitId, itemId, false]),
].flat();
await db.execute(
await connection.execute(
// TODO: When we start saving existing outfits, we'll need a delete
// here too, or some other sync mechanism.
`
@ -269,10 +270,12 @@ const resolvers = {
);
}
await db.commit();
await connection.commit();
} catch (e) {
await db.rollback();
await connection.rollback();
throw e;
} finally {
await connection.release();
}
console.info(`Saved outfit ${newOutfitId}`);