-
Notifications
You must be signed in to change notification settings - Fork 374
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.
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
[firestore-counter] Counter extension run locally: Transaction to aggregate shards failed. TypeError: Cannot read properties of undefined (reading 'length') #876
Comments
Instant temporary solution to bug report It seems that the worker array doesn't get initialized after the first iteration. To solve the error message and make the extension work properly replace the following line in the t.set(this.controllerDocRef, { timestamp: firebase_admin_1.firestore.FieldValue.serverTimestamp() }, { merge: true }); to if (controllerDoc.exists) { //only update timestamp
t.set(this.controllerDocRef, { timestamp: firebase_admin_1.firestore.FieldValue.serverTimestamp() }, { merge: true });
} else { //init worker list as well
t.set(this.controllerDocRef, { workers: [], timestamp: firebase_admin_1.firestore.FieldValue.serverTimestamp() }, { merge: true });
} I am not sure if I was missing something during the implementation, but it seems this error doesn't appear in the cloud?! Is this a known error? |
@joehan what do you think about this? |
Wooof, this is a tricky one. Glad to hear that you found a workaround, and AFAIK we've never seen this behavior in production. I spent some time digging through the code, and here's my theory on why you see it only in the emulator:
|
Give me a week for the longest, I will make it sure to notify you about the results. |
Hi @polarby, |
Thanks for recap. Should work now |
Same issue at Counter extension emulatorSame error is present, when using the new extension emulator (great implementation btw. !!!) Tested with Following specs:
I am not quite sure why this is happening. Can someone recheck that this is not only my device/config/etc..? Example Program:const admin = require("firebase-admin");
const db = admin.firestore();
const functions = require("firebase-functions");
const Counter = require("./counterConnector.js"); // javascript client class
exports.download = functions.https.onCall(async (data, context) => {
const counter = new Counter(db.doc("content/12345"), "downloads");
await counter.incrementBy(1);
// other code...
}); The counter javascript client can be found here |
Hi @polarby, |
Both ways! Either running the new extension emulator (new-way) or simply running the function on cloud functions (old-way).
It appears that certain triggers might be unsupported resulting in them not getting triggered and not initiating the worker's array. |
This is a proposed pull request for [this](firebase#876 (comment)) issue. Note that this was only tested on the js version, and was simply rewritten for ts purposes.
Take a look at the pull request I have just created. This should resolve the local emulator problems. Based on my understanding this change should not interfere with the cloud behavior, but this is NOT TESTED. There might be very well a better solution and my pull request might be just a quick fix for a more severe problem. |
EDIT: This seems to be a wrong report on my end (i cannot reproduce a resolvement of this issue) |
@polarby @yamankatby I see this error two with newest What is interesting is that the error only occurs on the second run of Here is an excerpt of the logs.
Why is it happening?As on the first run the controllerDoc does not exist at all, extensions/firestore-counter/functions/src/controller.ts Lines 144 to 154 in 6656cfe
However, later on in extensions/firestore-counter/functions/src/controller.ts Lines 204 to 207 in 6656cfe
On second run of extensions/firestore-counter/functions/src/controller.ts Lines 144 to 154 in 6656cfe
Why does the error not occur on the third run and afterwards?On the second run, the extensions/firestore-counter/functions/src/controller.ts Lines 152 to 156 in 6656cfe
This error gets caught by the surrounding catch clause .. extensions/firestore-counter/functions/src/controller.ts Lines 213 to 215 in 6656cfe
.. which forces the calling function to call extensions/firestore-counter/functions/src/index.ts Lines 40 to 48 in 6656cfe
.. which in the end adds an extensions/firestore-counter/functions/src/controller.ts Lines 307 to 309 in 6656cfe
Does it also occur outside the emulator?Yes, I was also able to find the error in my production function: Do we want to fix it?Indeed it is not a critical issue, because clever error handling prevents the issue from becoming critical. However, from a developer and customer experience it would be great if the error would not happen at all. @polarby Would you be able to reopen the issue? Tomorrow I could provide a minimal PR. |
Thanks for the further elaboration on this ongoing bug and interesting insight into errors also in production (i wasn't aware of this). I am sorry about the lack of examination on the previous issue closure on my end. |
Closing as this seems to be fixed by the merged PR? Happy to reopen if i'm mistaken |
Configuration
LOCATION="europe-west3"
INTERNAL_STATE_PATH= "firebase_ext/sharded_counter"
SCHEDULE_FREQUENCY= 1
Problem
This is a follow-up bug report to this question
During the Implementation of Distributed-Counter-Extension for local emulator I was able to run the scheduled function (
controllerCore();
) via the firebase shell and it will only work for the first aggregation. The next one will throw the following error:What I have encountered is, that when removing
_firebase_ext_
from the local firestore it will work again for one aggregation again (and the directory will be created again).When running the controllerCore again after the error I will get a timeout warning of my worker function.
Steps to reproduce:
Implement the Counter extension to the local emulator
with the following export changes in the index file as described here:
The text was updated successfully, but these errors were encountered: