DocumentReference.set() (other operations as well) can synchronously throw errors before returning a Promise.

[inf3rnus]

1. Is this a client library issue or a product issue?

This is the client library for firestore.

2. Did someone already solve this?

I don't believe so.

Environment details

• OS: Ubuntu 22..04
• Node.js version: 22.14.0
• npm version: 10.9.2
• firebase-admin version: 12.7.0

Steps to reproduce

Run this code:

const admin = require("firebase-admin");
admin.initializeApp();

const firestore = admin.firestore();

const sleep = timeToSleepMs =>
  new Promise(resolve => setTimeout(resolve, timeToSleepMs));

async function sleepAndThrow(sleepMs, message) {
  console.log(`Sleeping for: ${sleepMs}`);

  await sleep(sleepMs);

  console.log(`Done sleeping for ${sleepMs}`);

  try {
    throw new Error(message);
  } catch (error) {
    console.error(`In the func throwing: `, error);

    throw error;
  }
}

async function thisDoesNotWork() {
  const docRef = firestore.doc("someCollection/someDoc");

  try {
    await Promise.all([
      sleepAndThrow(
        200,
        "I am throwing, because Promise.all() is never called"
      ),
      docRef.set({
        hi: true,
        // throws an error from validateData()
        throwSyncOnUndefined: undefined
      })
    ]);
  } catch (error) {
    console.error(error);
  }

  await sleep(1e3);

  console.log(
    "This will never print, because .set() is sync code that does sync work before returning a promise"
  );
}

async function thisDOESWork() {
  const docRef = firestore.doc("someCollection/someDoc");

  try {
    await Promise.all([
      docRef.set({
        hi: true,
        // throws an error from validateData()
        throwSyncOnUndefined: undefined
      }),
      sleepAndThrow(
        200,
        "I am NOT throwing, because docRef.set() has thrown an error"
      )
    ]);
  } catch (error) {
    console.error(error);
  }

  await sleep(1e3);

  console.log(
    "This will print, because sleepAndThrow is never called, as docRef.set() throws an error synchronously before any further code can be called"
  );
}

async function main() {
  await thisDoesNotWork();
  await thisDOESWork();
}

main();

Run each of the functions in main() by themselves and see for yourselves. The first function does not work because the function throws synchronously after a promise has been added to the event loop. The second one works because the error is thrown before a promise can be added to the event loop.

Here's another example for further clarity:

async function thisDoesNotWork() {
  const docRef = firestore.doc("someCollection/someDoc");

  await docRef
    .set({
      hi: true,
      // throws an error from validateData()
      throwSyncOnUndefined: undefined
    })
    .catch(() => {
      console.error("I don't work");
    })
    .then(() => {
      console.log("I work!");
    });

  console.log(
    "This will never print, because .set() is sync code that does sync work before returning a promise"
  );
}

Does not work for the same reason.

It needs to be converted to this, and there's more than one way to do this, but:

async function thisDOESWork() {
  const docRef = firestore.doc("someCollection/someDoc");

  try {
    await docRef
      .set({
        hi: true,
        // throws an error from validateData()
        throwSyncOnUndefined: undefined
      })
      .catch(() => {
        console.error("I don't work");
      })
      .then(() => {
        console.log("I work!");
      });
  } catch (error) {
    console.error(error);
  }

  console.log(
    "I will print because I'm catching a sync error, even though it doesn't look like it"
  );
}

I can think of two solutions to this:

1. All methods that return a promise for DocumentReferences that perform sync work before returning a promise need to be wrapped in a promise, i.e. have the async keyword added to them. Or at least the wrappers that call those methods in firebase-admin/firestore need them. Or something to that effect.

2. Update the docs everywhere to indicate that it can throw synchronously... Which just feels... wrong.

Unless someone can find a way of how doing this would break code, I think option 1's the best bet.

And for the sake of entertaining the argument that your props should always be defined, yes your props should be never undefined, sure, but there are always bugs in software, bugs you don't expect to have happen.

So when your system inevitably has a bug arise that's the result of bleedover from a different failure, and the props end up being undefined, and when your catch block is not executed the way you think it will because you were expecting a promise to either resolve or reject, very bad things happen. e.g. server crashes that can leave data in an inconsistent state. If that data controls infrastructure, things get expensive and dangerous financially.

This does open a broader discussion around if this nuance of javascript makes sense from a DX perspective, there should be lint rules for this at a minimum. I actually feel like this is a gap in ECMA and could imagine a world where JS engines throw a static error before running the code if this situation is discovered.

Here's a link to the line of code that's the root of the issue:

nodejs-firestore/dev/src/write-batch.ts

Line 338 in d559080

const ref = validateDocumentReference('documentRef', documentRef);

What's everyone's take on this?

Best,
Aaron

[milaGGL]

Hi @inf3rnus , thank you for raising this issue. We appreciate your thorough analysis and proposed solutions.

The current behavior of throwing synchronous errors for input validation is actually an intended behaviour. This is based on our internal API design guidelines, specifically, the rule regarding input validation:
Input validation errors should be thrown synchronously at the time of the call. For asynchronous APIs, it means to throw immediately at the call-site, instead of rejecting or calling error() callback.

Thank you again for your feedback.

[inf3rnus]

@milaGGL Very interesting... Please take this as genuine curiosity, what's the rationale behind the specification?

Also, would you guys mind adding this as a task to have this detail added to the documentation?