Fix bad interaction between methods and buffering

[mitar]

So it seems we introduced a bug with #5680.

The issue is the following scenario:

• you have a publish sending changes to a field a
• they get buffered
• at the same time a client runs a method with a stub which changes field b
• method is simulated and ends
• buffered changes are applied
• server-side confirms a method
• client-side switches to a state after a simulated method call using replace message with the old a value, without that published change being present

Or something like this. It is really tricky race condition when this happens. But the solution is simple. Methods should always run on the latest state of data, without any pending buffered state. So this pull requests applies pending buffered state. In retrospective this looks obvious.

I am unsure how to make a test for this to discover such cases in the future with tests.

cc @nathan-muir

[mitar]

And do not ask how much debugging went into this one line code change.

[mitar]

I am not sure what you are asking.

[mitar]

My current workaround to fix this locally:

saveOriginals = Meteor.connection.__proto__._saveOriginals
Meteor.connection.__proto__._saveOriginals = ->
  @_flushBufferedWrites()
  saveOriginals.apply this, arguments

[mitar]

Hm, method call between reset and quiescence test is failing now.

[mitar]

Changing the line to:

    if (!self._waitingForQuiescence())
      self._flushBufferedWrites();

Seems to fix the test, but it is unclear if this is the right thing to do.

[ppotoplyak]

@mitar what are the symptoms of hitting this issue? Could this cause client side method callbacks to not fire? #7868

[mitar]

You can try this small patch and see if it fixes for you. But I do not think it is related.

Symptom here for me was that data on the client collection was old data because. So updates to the collection were overridden. I do not think this happens for methods because methods are not "updates".

[mitar]

I added a test.

[mitar]

Can we get some eyes on this? @glasser?

[glasser]

Gosh, it's been a long long time since I've spent any time in this codebase. @benjamn do you think I'm still the best person to review this?

[abernix]

I'm not sure I'm the best person to review this either, although the original description and the recently added test (18b5ad2) makes it fairly clear to me. There might be a better fix, but I'd need to do quite a bit more digging to know. I'm curious if we should change the test to use lolex like the livedata stub - buffering data test for precision timing on it, but perhaps it's not necessary.

I can confirm that the test fails without the "fix" code and I've gone ahead and rebased this off devel.

[benjamn]

I think self._waitingForQuiescence() is at least reliably correlated with the condition we need to protect against: that self._resetStores is true because we recently reconnected (in which case there shouldn't be any buffered writes to flush anyway). Given the thoroughness of the tests, I agree this change seems safe as-is.