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

after upgrading to 0.8.2 Collections are empty in helper method after an insert #2275

Closed
calculon0 opened this issue Jul 4, 2014 · 2 comments

Comments

@calculon0
Copy link

I'm running into weird behavior after upgrading to 0.8.2. After inserting an object into a Collection, a helper method tries to find that object in the Collection and display it in a template.

0.8.1.3 works fine. 0.8.2 has an empty Collection in the helper method.

repo: https://github.com/calculon0/empty-collection-in-helper

Clicking the button will add an object into the Collection and it will get displayed in 0.8.1.3. In 0.8.2 and devel, errors are thrown because the Collection is empty.

I narrowed it down to commit df2820f.

@ramishka
Copy link

ramishka commented Jul 7, 2014

Im experiencing the same behavior in a project of my own. I can recreate the behavior in the sample code by @calculon0 too.
I noticed that when the collection is being queried (i.e. during a render or a re-render), there is a point when it becomes empty. But it does get the proper record count back, in subsequent queries.
Same symptoms - works in 0.8.1.3 and broken in 0.8.2

You can see this in my sample project here https://github.com/ramishka/meteor-collection-test
Watch the console when adding a new record and monitor the collection size.

glasser added a commit that referenced this issue Jul 16, 2014
The minimongo calls that cared about waiting for observe callbacks to be
fired before returning use drain() on a Meteor._SynchronousQueue to do
that wait. But if drain is called from within a task on the same queue,
drain returned immediately, which failed to provide the desired
semantics.

This commit changes drain in this context to throw instead. This would
now make a reentrant observeChanges (or mutator) throw, so we fix that
by splitting up LocalCollection._observeQueue into two different pieces.

One piece is used purely for the above "drain" purpose: this is the
stack of queues LocalCollection._observeQueues. Each call to
observeChanges/insert/update/remove/resumeObservers gets a new queue
which it can drain, and since it's a new queue, the drain call is not
within a task on the *same queue*, so the error case doesn't occur.

However, just doing this would break the constraint that multiple
observe callbacks for the same observe are always called in
sequence (even if on the server, one yields). So we use a *separate*
queue, which is per-observe (and which never gets drained) for this
purpose. (This ensures that the test "mongo-livedata - minimongo observe
on server" still passes.)

Note that #2275 was made more obvious by df2820f, which caused the
common case of findOne inside a helper inside a `#each` to end up
dependent on the synchronous observeChanges contract. Reverting that
commit does fix our newly added test "minimongo - fetch in observe" but
it does not help "minimongo - observe in observe".

Fixes #2275.

....

....

except that this leads to the unacceptable behavior where the
newly-added-in-this-commit test "minimongo - insert in observe"
fails.

That's because fundamentally, the following things cannot all be
simultaneously true:

(a) For a given observeChanges calls, its callbacks will be invoked in
sequence; they cannot overlap either due to server-side yielding or due
to re-entrancy.

(b) When LocalCollection.insert() is called, all relevant observe
callbacks are invoked before it returns.

(c) It is legal to call insert (or another mutator) on a collection from
within an observe callback in such a way that it will affect the result
of the observed cursor.

This commit resolves this issue by making (c) illegal. But this is
probably a pretty valid use case. So we are not actually planning to
merge this commit.

Instead, we should probably relax (b): we should probably not drain the
queue anywhere but in observeChanges.  This would actually be consistent
with how it works on the server, where you have to use a write fence if
you'd like to wait for all observes to be called. This is a bigger
change than we have time for now, because we'd have to ensure that write
fences work on the client, make minimongo interact with them, and
rewrite all of our minimongo tests that care about observe timing to use
write fences (just like the mongo-livedata tests already do). So for now
we will probably "fix" #2275 in a more ad-hoc and targeted way.
@glasser
Copy link
Contributor

glasser commented Jul 16, 2014

This is a special case of #2315. We don't think we will try to fix #2315 right now, but we will try to fix #2275, probably by reverting df2820f and adding a few more tests and safeguards.

estark37 pushed a commit that referenced this issue Jul 21, 2014
This reverts commit c05ae24 EXCEPT for
the "fetch in observe" test, which I'm leaving in because we still want
a test for #2275.

This commit made it an error to start an observeChanges from within an
observeChanges callback for the same collection, but it turns out that
this is actually a somewhat common thing to do (for example, nested
'each'). Instead, we'll leave things the way they were pre-0.8.2: you
can start an observeChanges from within an observeChanges callback, but
it'll be subtly buggy in that you won't get synchronous 'added'
callbacks. This issue is described in #2315, along with the fact that
insert/update/remove/resumeObservers won't run their affected observe
callbacks if they are called from within a task on the collection's
queue. Eventually we'll implement the full fix (which relaxes the
requirement that insert/update/remove run all their callbacks before
returning) described in #2315.
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

No branches or pull requests

3 participants