From 3744a476e50c9a25d14604e9a723276bdb9676da Mon Sep 17 00:00:00 2001 From: Matchu Date: Fri, 7 Jan 2022 18:06:46 -0800 Subject: [PATCH] Fix db transactions with pooling 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.) --- src/server/types/Item.js | 29 +++++++++++++++---------- src/server/types/MutationsForSupport.js | 13 ++++++----- src/server/types/Outfit.js | 17 +++++++++------ 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/server/types/Item.js b/src/server/types/Item.js index 05ef9ed..343ec21 100644 --- a/src/server/types/Item.js +++ b/src/server/types/Item.js @@ -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; diff --git a/src/server/types/MutationsForSupport.js b/src/server/types/MutationsForSupport.js index edf9798..02f8e4f 100644 --- a/src/server/types/MutationsForSupport.js +++ b/src/server/types/MutationsForSupport.js @@ -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 diff --git a/src/server/types/Outfit.js b/src/server/types/Outfit.js index 99293c7..8af067c 100644 --- a/src/server/types/Outfit.js +++ b/src/server/types/Outfit.js @@ -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}`);