Skip to content
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

Stack overflow when collection is large and becomes unobserved #58

Open
damonmaria opened this issue Dec 2, 2018 · 11 comments
Open

Stack overflow when collection is large and becomes unobserved #58

damonmaria opened this issue Dec 2, 2018 · 11 comments
Labels

Comments

@damonmaria
Copy link
Contributor

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:

  • firestorter: 1.1.1
  • mobx: 5.6.0

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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

@IjzerenHein
Copy link
Owner

Cool!

@damonmaria
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@IjzerenHein
Copy link
Owner

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

@IjzerenHein IjzerenHein added the bug label Dec 3, 2018
@damonmaria
Copy link
Contributor Author

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
Copy link

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
Copy link

@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
Copy link

Any further update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants