IjzerenHein / firestorter

Use Google Firestore in React with zero effort, using MobX 🤘
http://firestorter.com
MIT License
378 stars 50 forks source link

Stack overflow when collection is large and becomes unobserved #58

Open damonmaria opened 5 years ago

damonmaria commented 5 years ago

If I have a lot of documents in a Collection I'm getting the following stack overflow when that collection becomes unobserved:

Uncaught RangeError: Maximum call stack size exceeded
    at MetricBatchDoc../node_modules/firestorter/lib/firestorter.module.js.Document.releaseObserverRef (firestorter.module.js:796)
    at ObservableValue$$1.atom.onBecomeUnobserved (firestorter.module.js:96)
    at endBatch$$1 (mobx.module.js:1419)
    at endAction (mobx.module.js:609)
    at executeAction$$1 (mobx.module.js:577)
    at runInAction$$1 (mobx.module.js:1875)
    at MetricBatchDoc../node_modules/firestorter/lib/firestorter.module.js.Document.releaseObserverRef (firestorter.module.js:800)
    at ObservableValue$$1.atom.onBecomeUnobserved (firestorter.module.js:96)
    at endBatch$$1 (mobx.module.js:1419)
    at endAction (mobx.module.js:609)
    at executeAction$$1 (mobx.module.js:577)
    at runInAction$$1 (mobx.module.js:1875)
    at MetricBatchDoc../node_modules/firestorter/lib/firestorter.module.js.Document.releaseObserverRef (firestorter.module.js:800)
    at ObservableValue$$1.atom.onBecomeUnobserved (firestorter.module.js:96)
    at endBatch$$1 (mobx.module.js:1419)
    at endAction (mobx.module.js:609)
    at executeAction$$1 (mobx.module.js:577)
    at runInAction$$1 (mobx.module.js:1875)
...

Although it mentions MetricBatchDoc (my own Document subclass) it can't be affecting it. The entire definition is:

export class MetricBatchDoc extends Document {
  public readonly data: IMetricBatch
}

This is after a yarn upgrade so it's the latest versions of everything:

To be certain of the 2 lines in Firestorter involved it's the runInAction line in this method:

    Document.prototype.releaseObserverRef = function () {
        var _this = this;
        if (this.isVerbose) {
            console.debug(this.debugName + " - releaseRef (" + (this.observedRefCount - 1) + ")");
        }
        var res = --this.observedRefCount;
        if (!res) {
            runInAction(function () { return _this._updateRealtimeUpdates(); });
        }
        return res;
    };

Called from this override of Mobx's Atom:

    atom.onBecomeUnobserved = function () {
        var res = onBecomeUnobserved.apply(atom, arguments);
        if (isObserved) {
            isObserved = false;
            delegate.releaseObserverRef();
        }
        return res;
    };

I'm pretty sure it's not infinite recursion because:

  1. If I reduce the number of documents in the collection it doesn't happen
  2. If I turn debug: true on then the console.debug statement in the above releaseObserverRef lists a different document each time

So it's like in the process of handling one document becoming unobserved it triggers another document to become unobserved, calling deeper, rather than them being processed linearly.

damonmaria commented 5 years ago

A further note to this. I tried to get around it by setting mode to off (on both the Collection and the Documents) but that doesn't solve the issue. Because even when both modes are off the Document still has snapshotObservable and dataObservable, despite the fact neither can change in that situation (if I understand it correctly). I wonder if things could be more efficient if the Documents created by a Collection with mode off weren't observable at all?

IjzerenHein commented 5 years ago

Hi, is there a way I could reproduce this? Can you maybe share an simple online example in StackBlitz which triggers this problem? cheers

damonmaria commented 5 years ago

I think it'll happen when too many docs in a collection. Will try to do small example online.

IjzerenHein commented 5 years ago

Cool!

damonmaria commented 5 years ago

@IjzerenHein: Here you go: https://stackblitz.com/edit/react-tzdzlo?file=Hello.js

Check the checkbox. Wait for it to update. Then uncheck it and you should get a stock overflow. I guess in Stackblitz you don't get proper stacktraces for some reason. But you should be able to replicate locally.

I've made one Collection in my app readable without auth to test this. I'd like to remove that permission soon so please post here when you have the info you need.

damonmaria commented 5 years ago

FYI, that collection has just over 2000 docs in it. I know that's excessive... but I think this is still a bug.

IjzerenHein commented 5 years ago

Hey, thanks for the online demo/test, that is very helpful. I've been looking into it but haven't found a solution yet. I noticed that there were some similar issues that people where having with certain mobx versions. I tried downgrading to mobx 4.7 and upgrading to 5.7, but that didn't solve the problem. I had a look at the code but haven't been able to reason about it why this is happening. I will need to take a closer look when I have more time. For now, I have added a unit-test based on your code and that now reproduces the same problem. To be continued and if you have any more info or hunches, let me know. cheers

damonmaria commented 5 years ago

Thanks for the update and your time. Since you now have a unit test to reproduce I'll disable public access to the collection.

samosaboy commented 5 years ago

Any update on this? We are using Firestorter too and experience this problem when we navigate away from a page that is rendering lots of documents inside a collection.

brkastner commented 4 years ago

@samosaboy I was able to work around this issue by extending Document & overriding the observerRef methods like so:

class DisabledDocument extends Document {
    addObserverRef(): number {
        return 0;
    }
    releaseObserverRef(): number {
        return 0;
    }
}

// tell firestorter to use our custom Document class when processing docs
const col = new Collection<DisabledDocument>('collection_path', {
    createDocument: (source, options) => new DisabledDocument(source, {...options, mode: Mode.Off})
});
AlbertInRC commented 1 year ago

Any further update?