Is it safe to call a throwing function inside a catch block?-CodePudding

exports.createUser = functions.https.onCall(async (data, _context) => {
    const email = data.email;
    const password = data.password;

    try {
        // First, create the user account.
        const userRecord = await admin.auth().createUser({
            email: email,
            password: password,
        });

        // User successfully created, now update the database.
        const userId = userRecord.uid;
        const db = admin.firestore();
        const batch = db.batch();
        batch.create(
            db.collection("userAccounts").doc(userId), {created: admin.firestore.FieldValue.serverTimestamp()},
        );
        await batch.commit();

        // Database successfully updated, now return the newly-created userId.
        return Promise.resolve(userId);
    } catch (error) {
        if (userId) {
            // There was an error updating the database. However, a user was created
            // beforehand. Therefore, delete the user before terminating the
            // function.
            admin.auth().deleteUser(userId); // <-- this throws
            throw new functions.https.HttpsError("unknown", "Database error", error);
        }
        throw new functions.https.HttpsError("unknown", "Authentication error", error);
    }
});

This function performs two async tasks: creates a user and updates a database. If the database update fails, I need to undelete the user. This function deletes the user in the catch block when it catches a database error. However, deleting the user itself throws. If the delete-user function fails, would that error be caught in this catch block and possibly create an infinite loop? How best to handle this scenario?

CodePudding user response：

It will not create an infinite loop, but the error won't be caught either - which should be avoided. You'll need to tack on another .catch, because you also want to throw a different error afterwards.

} catch (error) {
    if (userId) {
        // There was an error updating the database. However, a user was created
        // beforehand. Therefore, delete the user before terminating the
        // function.
        admin.auth().deleteUser(userId)
            .catch(() => {}); // Ignore errors thrown by deleteUser
        throw new functions.https.HttpsError("unknown", "Database error", error);
    }
    throw new functions.https.HttpsError("unknown", "Authentication error", error);
}

You should also be sure that the caller of this callback passed to onCall catches its asynchronous errors to avoid unhandled rejections.

Still, this process is pretty suspect - it seems odd to be deliberately calling a function you think will probably throw. If you need to call deleteUser only in the case that createUser succeeds, checking for the userId looks like the right approach, but that'll only work if the variable is in scope at that point, which it isn't. You can change your code to:

exports.createUser = functions.https.onCall(async (data, _context) => {
    const { email, password } = data;
    let userId; // <---------------------------
    try {
        // First, create the user account.
        const userRecord = await admin.auth().createUser({ email, password });

        // User successfully created, now update the database.
        userId = userRecord.uid; // <---------------------------
        const db = admin.firestore();
        const batch = db.batch();
        batch.create(
            db.collection("userAccounts").doc(userId), {created: admin.firestore.FieldValue.serverTimestamp()},
        );
        await batch.commit();

        // Database successfully updated, now return the newly-created userId.
        return userId; // don't need to wrap this in a Promise
    } catch (error) {
        if (userId) {
            // There was an error updating the database. However, a user was created
            // beforehand. Therefore, delete the user before terminating the
            // function.
            admin.auth().deleteUser(userId)
                .catch(() => {}); // This SHOULDN'T throw, but just in case
            throw new functions.https.HttpsError("unknown", "Database error", error);
        }
        throw new functions.https.HttpsError("unknown", "Authentication error", error);
    }
});

Or to

exports.createUser = functions.https.onCall(async (data, _context) => {
    const { email, password } = data;
    let userId;
    try {
        // First, create the user account.
        const userRecord = await admin.auth().createUser({ email, password });
        userId = userRecord.uid;
    } catch (error) {
        throw new functions.https.HttpsError("unknown", "Authentication error", error);
    }
    try {
        // User successfully created, now update the database.
        const db = admin.firestore();
        const batch = db.batch();
        batch.create(
            db.collection("userAccounts").doc(userId), { created: admin.firestore.FieldValue.serverTimestamp() },
        );
        await batch.commit();

        // Database successfully updated, now return the newly-created userId.
        return userId;
    } catch (error) {
        // There was an error updating the database. However, a user was created
        // beforehand. Therefore, delete the user before terminating the
        // function.
        admin.auth().deleteUser(userId);
        throw new functions.https.HttpsError("unknown", "Database error", error);
    }
});