Skip to content

synchronous return point for observeChanges/insert/etc doesn't work reentrantly #2315

Open
glasser opened this Issue Jul 16, 2014 · 10 comments

2 participants

@glasser
Meteor Development Group member
glasser commented Jul 16, 2014

In minimongo, several methods have a contract about what observe callbacks will be invoked before they synchronously return. For example, observeChanges invokes all the initial adds before returning; and insert/update/remove/resumeObservers invokes all affected observe callbacks before returning.

This is implemented by passing all observe callbacks through a Meteor._SynchronousQueue and calling drain() from within those functions.

However, drain() returns immediately when itself called from a task on the same queue (eg, if observeChanges or insert is called from an observe callback). That means that in this case, the methods don't wait at all like they should.

@glasser
Meteor Development Group member
glasser commented Jul 16, 2014

We could try to fix this by having each one of those methods use its own queue but that leads to other issues (eg, we lose the requirement that all the observe callbacks on the same observe are called in sequence). We can fix this (see 90dd0fb on unpalatable-reentrant-minimongo-fix) by adding another per-observe queue, but that makes us need to outlaw mutations from within observe callbacks which affect the observe in question.

The best fix is probably unpalatable-reentrant-minimongo-fix (which addresses reentrant observeChanges and allows us to keep df2820f) but also completely dropping the requirement that mutation methods call their callbacks before returning (which is consistent with the server behavior). But that will require us to add write fence support on the client, at least for the sake of our test suite.

@glasser glasser added a commit that referenced this issue Jul 16, 2014
@glasser glasser Test that reverting df2820 fixed #2275.
Make some instances of #2315 into errors instead of silent early
returns.

Specifically, observeChanges calls (with added or addedBefore callbacks)
from within another observeChanges callback on the same collection will
be unable to differentiate between initial and later added/addedBefore
calls, which is serious enough to be an error (see #2315 for details).

We don't currently think that the other effect of #2315 (where observe
callbacks triggered by insert/remove/update/resumeObservers will not
occur before those methods return, if they are called reentrantly) is
problematic enough to deserve this sort of error.
c05ae24
@estark37 estark37 added a commit that referenced this issue Jul 21, 2014
@estark37 estark37 Revert part of "Test that reverting df2820 fixed #2275."
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.
78d08b5
@mitar
mitar commented Jan 17, 2015

How is this progressing?

@glasser glasser added this to the Winter 2015 Mongo Maintenance milestone Jan 21, 2015
@glasser
Meteor Development Group member
glasser commented Feb 20, 2015

Hoping to work on it soon if time allows. Of course as you know there are lots of Mongo-related issues worth looking at :)

@glasser
Meteor Development Group member
glasser commented Mar 6, 2015

Unfortunately, it doesn't look like I got to this as part of the 1.0.4 maintenance work.

@mitar
mitar commented Mar 6, 2015

Noooo. :-( Ah, why I reported all those nonimportant Minimongo stuff when this is the biggest issue.

@glasser
Meteor Development Group member
glasser commented Mar 6, 2015

I'm curious what your reason for caring about this one is — is it mostly what you discussed on #3597 about how it makes client and server less isomorphic?

@glasser
Meteor Development Group member
glasser commented Mar 6, 2015

Or I guess mostly because of #2691?

@mitar
mitar commented Mar 6, 2015

It is #2691. We use global observes on the client which call into complex code to react to changes to data. But now when we use latency compensation on the client, that complex code is failing regularly when there is this leaking of the context mentioned above. So this is failing in the current codebase. (Workarounds are using setTimeout (not Meteor.setTimeout) to escape the context. And also to use Tracker.nonReactive. But this is really ugly and it makes things unclear why are necessary.)

#3597 is something we are not yet using, as it is not yet fixed. That is more for the future. I have an initial implementation of server-side Tracker but I stopped working on that because I am waiting for these fixes first. But this is not yet so critical. It is more just to report everything so that things will get fixed eventually.

@mitar
mitar commented Mar 6, 2015

So the main issue is that the observe callback gets called in the context where insert was called on the client (#2691). The order of things (#3597) is less important.

@mitar
mitar commented Mar 22, 2015

My temporary workaround:

fixCallbacks = (callbacks) ->
  for own name, callback of callbacks
    do (name, callback) ->
      callbacks[name] = (args...) ->
        Tracker.nonreactive ->
          alreadyInSimulation = Package.ddp.DDP._CurrentInvocation.get()?.isSimulation
          unless alreadyInSimulation
            callback args...
          else
            Package.ddp.DDP._CurrentInvocation.withValue null, ->
              beforeStores = _.keys Meteor.connection._stores
              callback args...
              newStores = _.omit Meteor.connection._stores, beforeStores
              for name, store of newStores
                store.saveOriginals()

originalObserveChanges = LocalCollection.Cursor::observeChanges
LocalCollection.Cursor::observeChanges = (callbacks) ->
  fixCallbacks callbacks

  originalObserveChanges.call @, callbacks

originalObserve = LocalCollection.Cursor::observe
LocalCollection.Cursor::observe = (callbacks) ->
  fixCallbacks callbacks

  originalObserve.call @, callbacks

Not sure if instance context (Template.instance()) and other Blaze related contexts leak as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.