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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aff9063
Track computations from autoruns and ensure they're all stopped on pu…
etyp Sep 7, 2018
806a422
Add tests to guarantee unblock doesnt cause multiplexer leak
etyp Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ Package.onTest(function (api) {
api.use([
'peerlibrary:[email protected]',
'peerlibrary:[email protected]',
'peerlibrary:[email protected]'
'peerlibrary:[email protected]',
'meteorhacks:unblock'
]);

api.add_files([
Expand Down
8 changes: 8 additions & 0 deletions server.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,12 @@ extendPublish (name, publishFunction, options) ->
return

handles = []
trackedComputations = []
# This autorun is nothing special, just that it makes sure handles are stopped when publish stops,
# and that you can return cursors from the function which would be automatically published.
publish.autorun = (runFunc) ->
handle = Tracker.autorun (computation) ->
trackedComputations.push computation
result = runFunc.call publish, computation

collectionNames = getCollectionNames result
Expand Down Expand Up @@ -194,6 +196,12 @@ extendPublish (name, publishFunction, options) ->
while handles.length
handle = handles.shift()
handle?.stop()
# Stop computations in case a subscription is stopped before ready on the client and
# the publish is using meteorhacks:unblock's "this.unblock()". Without this, observers
# 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 •
edited
Loading

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.


result = publishFunction.apply publish, args

Expand Down
45 changes: 45 additions & 0 deletions tests.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
allCollections = []

for idGeneration in ['STRING', 'MONGO']
do (idGeneration) ->
if idGeneration is 'STRING'
Expand All @@ -8,9 +10,13 @@ for idGeneration in ['STRING', 'MONGO']
new Meteor.Collection.ObjectID()

Users = new Mongo.Collection "Users_meteor_reactivepublish_tests_#{idGeneration}", {idGeneration}
allCollections.push Users
Posts = new Mongo.Collection "Posts_meteor_reactivepublish_tests_#{idGeneration}", {idGeneration}
allCollections.push Posts
Addresses = new Mongo.Collection "Addresses_meteor_reactivepublish_tests_#{idGeneration}", {idGeneration}
allCollections.push Addresses
Fields = new Mongo.Collection "Fields_meteor_reactivepublish_tests_#{idGeneration}", {idGeneration}
allCollections.push Fields

if Meteor.isServer
LocalCollection = new Mongo.Collection null, {idGeneration}
Expand Down Expand Up @@ -244,6 +250,19 @@ for idGeneration in ['STRING', 'MONGO']

@ready()

Meteor.publish "unblocked-users-posts_#{idGeneration}", (userId) ->
@unblock()

@autorun (computation) =>
user = Users.findOne userId,
fields:
posts: 1

Posts.find(
_id:
$in: user?.posts or []
)

methods = {}
methods["insertPost_#{idGeneration}"] = (timestamp) ->
check timestamp, Number
Expand Down Expand Up @@ -883,5 +902,31 @@ for idGeneration in ['STRING', 'MONGO']
@assertEqual LocalCollection.find({}).count(), 15
]

multiplexerCountBefore = 0
multiplexerCountAfter = 0
@unblockedPub: (publishName) -> [
@runOnServer ->
multiplexerCountBefore = 0
for collection in allCollections
if collection
multiplexerCountBefore += Object.keys(collection._driver.mongo._observeMultiplexers).length
->
@userId = generateId()
handle = @subscribe "#{publishName}_#{idGeneration}", @userId
subscriptionId = handle.subscriptionId
handle?.stop()

Meteor.setTimeout @expect(), 1000 # ms
,
@runOnServer ->
multiplexerCountAfter = 0
for collection in allCollections
if collection
multiplexerCountAfter += Object.keys(collection._driver.mongo._observeMultiplexers).length
@assertEqual multiplexerCountBefore, multiplexerCountAfter
]

testClientUnblockedBasicAutorun: @unblockedPub 'unblocked-users-posts'

# Register the test case.
ClassyTestCase.addTest new ReactivePublishTestCase()