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

Fix method calls during reconnect #5104

Merged
merged 1 commit into from Sep 9, 2015
Merged

Conversation

@OleksandrChekhovskyi
Copy link

@OleksandrChekhovskyi OleksandrChekhovskyi commented Sep 4, 2015

User could enter irrecoverable broken state after doing any action while
reconnecting (specifically between stream reset and quiescence).
In this state some or all documents in collections would be missing,
because the loop processing buffered messages is interrupted with an
exception in _process_added.

This commonly happened on slow/bad networks, such as mobile.

@glasser
Copy link
Member

@glasser glasser commented Sep 8, 2015

Hi @OleksandrChekhovskyi! I think you've correctly identified a real and serious bug, and written a good fix and great test.

But I don't think your fix has the ideal semantics. Helpfully, I can point exactly to where it differs from what I think it should do in your test!

When a stub writes to a document, we're supposed to continue to see the stub value until the method returns. So I think your addition of _pushUpdate in this case is wrong, and at line 1853 we should still be seeing a value of 222.

Maybe to make your test more complete you could (a) add an assertion that the value becomes 222 as soon as the conn.call is done and (b) make the server send back 333 instead of 222 at the end, and we can see that the value goes from 111 before the call to 222 after the call to 333 after the method is done without ever "flickering" back to 111. (And I think the proper change to the actual code is just to not throw the error if _resetStores is set but otherwise still set the server document and not push the update for immediate application.)

@glasser glasser added the Project:DDP label Sep 8, 2015
@OleksandrChekhovskyi OleksandrChekhovskyi force-pushed the OleksandrChekhovskyi:devel branch Sep 9, 2015
@OleksandrChekhovskyi
Copy link
Author

@OleksandrChekhovskyi OleksandrChekhovskyi commented Sep 9, 2015

I've updated the patch to ensure that stub-written values survive reset.
It's a bit more complicated than what you've described though. _pushUpdate call has to be there, otherwise the store reset will simply remove the document. So to keep stub-written values, we still push the update, but with current document's fields.
Also added more comments in the code to clarify details.

Oleksandr Chekhovskyi
User could enter irrecoverable broken state after doing any action while
reconnecting (specifically between stream reset and quiescence).
In this state some or all documents in collections would be missing,
because the loop processing buffered messages is interrupted with an
exception in _process_added.

This commonly happened on slow/bad networks, such as mobile.
@OleksandrChekhovskyi OleksandrChekhovskyi force-pushed the OleksandrChekhovskyi:devel branch to c6ea93f Sep 9, 2015
test.isTrue(coll.findOne('aaa').value == 222);

stream.receive({msg: 'changed', collection: collName,
id: 'aaa', fields: {value: 333}});

This comment has been minimized.

@glasser

glasser Sep 9, 2015
Member

maybe even verify that this message isn't what changes it (though this isn't relevant to your fix)

@glasser
Copy link
Member

@glasser glasser commented Sep 9, 2015

OK, this looks good. I'm a little concerned about what happens if the method returns before the subs all get ready (ie, before quiescence) but that can be a later issue to fix...

glasser added a commit that referenced this pull request Sep 9, 2015
Fix method calls during reconnect
@glasser glasser merged commit 2be655d into meteor:devel Sep 9, 2015
1 check passed
1 check passed
CLA Author has signed the Meteor CLA.
Details
@dgreensp
Copy link
Contributor

@dgreensp dgreensp commented Sep 10, 2015

@OleksandrChekhovskyi, thank you for this PR!

@OleksandrChekhovskyi
Copy link
Author

@OleksandrChekhovskyi OleksandrChekhovskyi commented Sep 10, 2015

I'm a little concerned about what happens if the method returns before the subs all get ready

That should ideally be tested as well, but right now it's a pure theoretical concern, as in practice the method won't even be sent until quiescence (blocked by login wait method, injected from onReconnect callback).

@sasikanth513
Copy link

@sasikanth513 sasikanth513 commented Oct 8, 2015

we're getting the error server sent add for exsting item id:: xxxx most of the times when we disconnect and reconnect again and most of the times our app crashes because of this issue.

Hoping this PR will solve that issue. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.