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

Track computations from autoruns and ensure they're all stopped on publish stop #38

Merged
merged 2 commits into from
Sep 9, 2018

Conversation

etyp
Copy link
Contributor

@etyp etyp commented Sep 7, 2018

Fixes #37

# created from cursors inside the autorun will exist forever.
while trackedComputations.length
computation = trackedComputations.shift()
computation?.stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is strange that this helps you, because handle is the same as computation and I already stop them? So the whole idea why I collect handles there is to clean them up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is not the same, but then this is very strange and very problematic because it might point to a bug in server-side tracker implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitar would a reproduction repo help? I think the issue is that the computation isn't cleaned up ever because the publication isn't active nor is a computation returned from my autorun in this case. If I return a computation it does work, but can't do that since the pub needs to return a cursor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well your current implementation works only if a computation is returned from the autorun, but not if a cursor is.

Given meteorhacks:unblock is the only way to reproduce this, it seems like a race condition but I really don't know.

Copy link
Member

@mitar mitar Sep 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well your current implementation works only if a computation is returned from the autorun, but not if a cursor is.

No, result of calling Tracker.autorun is always a handle/computation of this autorun. It does not matter what internally is returned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure why you want to use unblock, BTW. There are some strange side effects and it uses a global variable I am pretty scared of seeing.

@mitar
Copy link
Member

mitar commented Sep 7, 2018

Also, please add a test which deterministically shows this problem and then how it is fixed.

@etyp
Copy link
Contributor Author

etyp commented Sep 8, 2018

I've written some tests to ensure Meteor's multiplexers aren't left hanging around. Originally my approach was to track the computations from the subscription in the test, but I found it much cleaner to do a count of the multiplexers before + after to prove it no longer leaks.

If you comment out computation?.stop() in server.coffee you'll see the multiplexer count never go down in tests.

Let me know your thoughts - happy to change approach if you think there's a better way.

@mitar
Copy link
Member

mitar commented Sep 9, 2018

I think I found the problem. I will merge this and update stuff a bit.

Thanks for finding this, reporting it, and making a PR.

@mitar mitar merged commit 0678258 into peerlibrary:master Sep 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants