Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix race condition in trigger disable #2858

Merged
merged 13 commits into from
Nov 25, 2020
Merged

Fix race condition in trigger disable #2858

merged 13 commits into from
Nov 25, 2020

Conversation

samtstern
Copy link
Contributor

@samtstern samtstern commented Nov 23, 2020 •
edited

Description

Fixes #2857

We give all background triggers a "generation number" which allows us to disable triggers inside the functions emulator and then when incoming calls try to run those triggers, even after a large delay, we can ignore it.

Scenarios Tested

functions/index.js

const functions = require('firebase-functions');

exports.httpFunction = functions.https.onRequest((request, response) => {
  console.log("httpFunction");
  response.send("Hello from Firebase!");
});

exports.rtdbFunction = functions.database.ref('/foo/{bar}').onWrite((change, ctx) => {
  console.log("rtdbFunction: ", ctx.params.bar);
  return true;
});

exports.firestoreFunctions = functions.firestore.document('/foo/{bar}').onWrite((change, ctx) => {
  console.log("firestoreFunction: ", ctx.params.bar);
  return true;
});

functions/test.js

const firebase = require("@firebase/rules-unit-testing");

async function addDoc(fs, id) {
  await fs.collection('foo').doc(id).set({
    date: new Date().getTime()
  });
  console.log("Added: ", id);
}

async function main() {
  const app = firebase.initializeAdminApp({
    projectId: 'fir-dumpster'
  });
  const fs = app.firestore();

  await addDoc(fs, 'should-trigger-1');
  await firebase.withFunctionTriggersDisabled(async () => {
    await addDoc(fs, 'should-not-trigger');
  });
  await addDoc(fs, 'should-trigger-2');
}

main();

Logs:

$ firebase emulators:exec "node test.js"                      
i  emulators: Starting emulators: functions, firestore, database
⚠  hub: emulator hub unable to start on port 4400, starting on 4401 instead.
⚠  functions: The following emulators are not running, calls to these services from the Functions emulator will affect production: auth, hosting, pubsub
⚠  Your requested "node" version "12" doesn't match your global version "10"
⚠  firestore: Did not find a Cloud Firestore rules file specified in a firebase.json config file.
⚠  firestore: The emulator will default to allowing all reads and writes. Learn more about this option: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration.
i  firestore: Firestore Emulator logging to firestore-debug.log
⚠  database: Did not find a Realtime Database rules file specified in a firebase.json config file. The emulator will default to allowing all reads and writes. Learn more about this option: https://firebase.google.com/docs/emulator-suite/install_and_configure#security_rules_configuration.
i  database: Database Emulator logging to database-debug.log
i  functions: Watching "/private/var/folders/xl/6lkrzp7j07581mw8_4dlt3b000643s/T/tmp.EtFy933F/functions" for Cloud Functions...
✔  functions[httpFunction]: http function initialized (http://localhost:5001/fir-dumpster/us-central1/httpFunction).
✔  functions[rtdbFunction]: database function initialized.
✔  functions[firestoreFunctions]: firestore function initialized.
i  Running script: node test.js
Added:  should-trigger-1
i  emulators: Disabling Cloud Functions triggers, non-HTTP functions will not execute.
i  functions[rtdbFunction]: function temporarily disabled.
i  functions[firestoreFunctions]: function temporarily disabled.
Added:  should-not-trigger
i  emulators: Enabling Cloud Functions triggers, non-HTTP functions will execute.
i  functions: Beginning execution of "firestoreFunctions"
✔  functions[rtdbFunction]: database function initialized.
✔  functions[firestoreFunctions]: firestore function initialized.
Added:  should-trigger-2
>  firestoreFunction:  should-trigger-1
i  functions: Finished "firestoreFunctions" in ~1s
i  functions: Beginning execution of "firestoreFunctions"
>  firestoreFunction:  should-trigger-2
i  functions: Finished "firestoreFunctions" in ~1s
✔  Script exited successfully (code 0)
i  emulators: Shutting down emulators.
i  functions: Stopping Functions Emulator
i  database: Stopping Database Emulator
i  firestore: Stopping Firestore Emulator
i  hub: Stopping emulator hub

Sample Commands

N/A

@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Nov 23, 2020
@samtstern

This comment has been minimized.

Sign in to view
@samtstern samtstern requested review from mbleigh and removed request for yuchenshi and mbleigh November 24, 2020 23:47
@samtstern

This comment has been minimized.

Sign in to view
@samtstern
Copy link
Contributor Author

@yuchenshi this one is ready to go now.

yuchenshi
yuchenshi approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@yuchenshi yuchenshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One question: What happens if the functions code is hot-reloaded while trigger is disabled? Would it cause a race condition somehow and re-enable the functions early?

@samtstern
Copy link
Contributor Author

@yuchenshi hot reload would not re-enable existing functions because the generation number won't be increased.

@samtstern samtstern merged commit b728a0f into master Nov 25, 2020
@samtstern samtstern mentioned this pull request Nov 25, 2020
Closed
@bkendall bkendall deleted the ss-trigger-disable-race branch August 4, 2021 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuchenshi yuchenshi yuchenshi approved these changes

Assignees

@yuchenshi yuchenshi

Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The new feature withFunctionTriggersDisabled not really disable the triggers
2 participants
@samtstern @yuchenshi