Skip to content

Commit

Permalink
Fix NPE in finishGame
Browse files Browse the repository at this point in the history
Apparently, transaction handlers are very often called with null even if
the object exists as an optimization. See:

https://groups.google.com/g/firebase-talk/c/Lyb0KlPdQEo
https://stackoverflow.com/a/65415636/5190601

Also, use server-side increment in createGame.

Fixes: #4
  • Loading branch information
eltoder committed Nov 18, 2024
1 parent 998b557 commit 15ee6a9
Showing 1 changed file with 14 additions and 29 deletions.
43 changes: 14 additions & 29 deletions functions/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,6 @@ export const finishGame = functions.https.onCall(async (data, context) => {
.ref(`gameData/${gameId}`)
.once("value");
const gameSnap = await admin.database().ref(`games/${gameId}`).once("value");
if (!gameSnap.exists()) {
throw new functions.https.HttpsError(
"not-found",
`The game with gameId ${gameId} was not found in the database.`
);
}

const gameMode = (gameSnap.child("mode").val() as GameMode) || "normal";

const { lastSet, deck, finalTime, scores } = replayEvents(gameData, gameMode);
Expand All @@ -73,6 +66,14 @@ export const finishGame = functions.https.onCall(async (data, context) => {
.database()
.ref(`games/${gameId}`)
.transaction((game) => {
// Transaction handler should always handle null, because firebase often
// calls it like that: https://stackoverflow.com/a/65415636/5190601
if (game === null) {
throw new functions.https.HttpsError(
"not-found",
`The game with gameId ${gameId} was not found in the database.`
);
}
if (game.status !== "ingame") {
// Someone beat us to the atomic update, so we cancel the transaction.
return;
Expand Down Expand Up @@ -290,30 +291,14 @@ export const createGame = functions.https.onCall(async (data, context) => {
// We update the database asynchronously in three different places:
// 1. /gameData/:gameId
// 2. /stats/gameCount
// 3. /publicGames (if access is public)
const updates: Array<Promise<any>> = [];
updates.push(
admin.database().ref(`gameData/${gameId}`).set({
deck: generateDeck(),
})
);
updates.push(
admin
.database()
.ref("stats/gameCount")
.transaction((count) => (count || 0) + 1)
);
// 3. /publicGames/:gameId (if access is public)
const updates: { [key: string]: any } = {};
updates[`gameData/${gameId}`] = { deck: generateDeck() };
updates["stats/gameCount"] = admin.database.ServerValue.increment(1);
if (access === "public") {
updates.push(
admin
.database()
.ref("publicGames")
.child(gameId)
.set(snapshot?.child("createdAt").val())
);
updates[`publicGames/${gameId}`] = snapshot.child("createdAt").val();
}

await Promise.all(updates);
await admin.database().ref().update(updates);
return snapshot.val();
});

Expand Down

0 comments on commit 15ee6a9

Please sign in to comment.