forEach vs For loop - Firebase batch.commit error

tutorials
#1

Hi there,

Just wanted to share an hard bug to track - but solved - eventually through a hint on JavaScript Loops - Code This, Not That - YouTube

I ran into issues building a batch write in Firebase. I used a forEach on an array to build a batch of writes, but the error said : you cannot write to batch already committed.

As I was building batches on batches, and at first glance, it seemed to appear randomly - it was driving me crazy.

Eventually, the solution was simple - use for of loop to iterate through array, not forEach. Then there is no async issue.

Just wanted to share, as this was a very hard one to find. Very deep in async-hell!

#2

Without any actual code, there’s a lot left to interpretation here, but I think there should be a way to reconcile the async nature of your tasks with whatever way you want to express the iteration.

Personally, when I hear the word “build” the way you used it, I would reach for Array.map. I would not use the word “build” to describe how I use forEach: forEach to me is about doing, as opposed to building.

Are you sure your underlying problem isn’t Yet Another Thing JavaScript Is Terrible At: variable scoping? I’m referring to something like the problem described here.

#3

Thx,

good point.

I had a service provider that does a forEach looping through a array. Each array item itself had an array which needed looping through.

Before everything, the batch was created, then the batch was filled in the inner array. And lastly after completing of the array, the batch was committed.

Using forEach this gave the error as reported in the last link, but not caused by same reason (I think). In a for of loop, everything stayed nice and dandy.

In the Fireship link, the speaker was talking (cant remember exactly) that a for loop behaves differntly then a forEach, which can cause issues in async. I would have to look that up.

New situation: (to explain the intended logic)

  async syncAll() {
    const batch = this.firebase.batch();

    console.warn('%c---------------- syncAll start', 'color:yellow');
    for (const provider of this.stateProviderList) {
      console.warn('%csyncAll for loop start', 'color:yellow', provider.getUniqueKey());
      await provider.syncState(batch);
      console.warn('%csyncAll for loop end', 'color:green', provider.getUniqueKey());
    }
    console.warn('%c---------------- syncAll end', 'color:green');

    return batch.commit()
      .then(res => {
        console.log('syncAll syncAll batch result OK', res);
      })
      .catch(res => {
        console.error('syncAll syncAll batch error', res);
      });
  }

And the syncState now has a for loop that eventually does:

batch.set(this.firebase.doc(this.firebaseStatePath + id).ref, newState.entityMap[id]);

#4

In fact, now I am explaining it, the outer loop was a Promise.all with an inner forEach. Not two forEach

Maybe I misunderstood the Promise.all…

Either way, the new way seems more predictable

#5

This was happening:

Async function calls in forEach

https://www.hacksparrow.com/javascript/foreach-in-promise-async-function.html

And maybe on a more conceptual level, I am leaning towards imperative thinking, instead of functional. And mixing can give issues?

1 Like
#6

No, I don’t think this one can be blamed on you. As with OO stuff, the concurrency in JavaScript is fake and bolted on, as opposed to having proper language support. I still prefer avoiding the async/await keywords because they obscure what is actually happening.

async and await rely on the cooperation of code that gets silently added underneath you, and forEach isn’t cooperating. What I think would work (but admittedly does not feel “authentic” enough to justify going away from for...of) would be something like this:

await Promise.all(tasks.map(task => thingReturningPromise(task)));

Note: “work” in the sense that once that line finishes, all your long-running tasks are done. It still doesn’t guarantee strict sequential execution. If you need strict control over the order, there’s a (again, very hard to read if you’ve never seen the idiom before) trick using reduce:

tasks.reduce((lastprom, task) => lastprom.then(() => thingReturningPromise(task), Promise.resolve);

I think of it as either a cousin of how compilers unroll loops or a matryoshka.

#7

Thx for the consideration and hints. Glad to see I am not completely mad, it is just the depth/complexity/counter-intuitive aspects of JS.

I do need control over the order of things as it is about an offline first app that needs to sync before doing a signout/commit. A Promise.All could potentially do, but I want to do some logging and somehow never have been friends with Promise.all.

The reduce example is one to print out and hang above my bed - maybe one day, I will be smart enough to see the simplicity of it :slight_smile:

Have a good one!

#8

Unique amongst Array utilities, reduce allows each iteration to leave a note for the next, like shift change notes in a factory. The graveyard shift supervisor can write something like “our shift ended before we could finish the ACME order - it’s almost ready for shipping on the desk over there”, and the day shift supervisor knows to do that before starting on ordinary day shift business. Day shift leaves a note for evening shift about what evening shift should start off doing, and night shift leaves a similar note for graveyard, etc.

In this case, the shift change note is the Promise representing the conclusion of work that that shift did. Next shift must wait on it to complete, then return a similar Promise for the next shift. The final kickoff Promise.resolve tells the first shift “hey, clean slate, just start doing what you need to do”.

1 Like
#9

Thank you. I appreciate the explanaition. Always a pleasure to learn here!