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

Buffered livedata fix #167

Merged
merged 3 commits into from
Apr 21, 2016
Merged

Conversation

abernix
Copy link
Contributor

@abernix abernix commented Apr 21, 2016

Since meteor/meteor#5680 provides (awesome) support for buffered DDP, two (what seem to be) false-positive issues popped up in fast-render. Nothing seemed actually wrong unless you were trying to do things mid-CPU tick (like tests or error checking). Both of these were found during accidental release of that PR in Meteor 1.3.2.1/2. The PR will be back in Meteor 1.3.3 and this should hopefully address the problems.

  1. flow-router started throwing a false-positive You can't use reactive data sources like Session inside the '.subscriptions' method error (despite the lack of subscriptions) because of the lack of flushing on the same tick in FastRender.init. Calling the connection's _flushBufferedWrites (if it exists), fixed this issue with no changes necessary to flow-router. The (incorrectly) thrown error was affecting subscriptions ability to become ready because the Tracker.
  2. fast-render package tests started failing because livedata updates were not happening in the same tick as the test that was checking them. I changed the tests that do this to use Tinytest.addAsync and wait until the default _bufferedWritesInterval milliseconds have passed. This value is hardcoded to 5 in livedata, so I used that here too.

Fixes #166

abernix added 3 commits April 21, 2016 19:16
...As of meteor/meteor#5680.  This can be left as it was though, and seems to work fine either way, though not overriding _waitingForQuiescence would be nice.
@abernix
Copy link
Contributor Author

abernix commented Apr 21, 2016

Should be completely backwards compatible too!

@arunoda
Copy link
Contributor

arunoda commented Apr 21, 2016

Hey. Thanks for the awesome fix.

@arunoda arunoda merged commit 7af4a0f into kadirahq:master Apr 21, 2016
@arunoda
Copy link
Contributor

arunoda commented Apr 21, 2016

Published as v2.14.0. Will do a releases to FlowRouter as well.

@tmeasday tmeasday mentioned this pull request Aug 10, 2016
conn._waitingForQuiescence = function() {return false};
conn._livedata_data(message);
conn._waitingForQuiescence = originalWait;
if (conn._bufferedWrites) {
Copy link

@tmeasday tmeasday Aug 10, 2016

Choose a reason for hiding this comment

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

I think this change should not have been included because if the connection is _waitingForQuiescence (for instance if a login method is run on startup), the writes get placed into a _messagesBufferedUntilQuiescence queue.

Those messages will not get into the _bufferedWrites queue until quiescence happens (the method returns from the server). So the later _flushBufferedWrites() will not pick them up.

See (meteor/meteor#7618)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why I had things working at some point, but indeed, that makes sense. Looks like @dnish already got the fix applied for this. Good catch!

@abernix abernix deleted the buffered-livedata-fix branch September 16, 2016 12:19
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.

Meteor 1.3.2.1 - Breaking Changes to Fast Render
3 participants