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

Latency compensation doesn't work properly #51

Closed
maxnowack opened this issue Nov 23, 2016 · 21 comments
Closed

Latency compensation doesn't work properly #51

maxnowack opened this issue Nov 23, 2016 · 21 comments
Labels

Comments

@maxnowack
Copy link
Contributor

As mentioned in this post, I've noticed an issue with latency compensation / optimistic ui.
The changed documents will be synced to the client, after the result of a method was received. That causes some flickering. The method on the client side will be reverted because the changes were not sent by the server before the method result was received.
The normal behavior is, that the changes were synced, while the client is waiting for the method result. After the result was received, the client compares invalidates the local working copy (minimongo) and takes the current state from the server.

DDP behavior with redis oplog:
image

Normal DDP behavior:
image

@maxnowack maxnowack changed the title Latency compensation doesn't work Latency compensation doesn't work properly Nov 23, 2016
@theodorDiaconu
Copy link
Contributor

Ok so let me think, I saw that you got most of the code from here:
https://github.com/meteor/meteor/blob/devel/packages/allow-deny/allow-deny.js#L356

And did the inserts on the collection not collection._collection.

Ok I get it so the problem is that it causes flickering, but in the end, the local state is the correct one ?

@theodorDiaconu
Copy link
Contributor

theodorDiaconu commented Nov 24, 2016

@maxnowack just tested this locally and it works for me without a problem. Client-side inserts are updated properly. (without using your package)

@maxnowack
Copy link
Contributor Author

hmm, I got the same issue without my package and normal method calls:
image

@theodorDiaconu
Copy link
Contributor

@maxnowack then it is not related to client-side inserts. good we figured that out.

But what is the reason, you do a call, the local side seems to be updated. What's the exact issue ?

@theodorDiaconu
Copy link
Contributor

@maxnowack for latency compensation you will need to do Meteor.apply('name', [args], options)

@maxnowack
Copy link
Contributor Author

I think this demo explains the issue:
redisoplog_lc
As you can see, the post was added, then removed and then readded.
Here's the code: https://github.com/maxnowack/meteor-redisoplog-latencycompensation

@maxnowack
Copy link
Contributor Author

@theodorDiaconu why do I need to use Meteor.apply for latency compensation? It is sufficient, if the method was defined on client and server, isn't it?

@theodorDiaconu
Copy link
Contributor

theodorDiaconu commented Nov 24, 2016

@maxnowack no, that's not how it works, you need to do apply, and put options like returnStubValue, throwStubExceptions. Thing is optimistic ui is very badly documented. I am now cloning the repo and trying with apply :)

@maxnowack
Copy link
Contributor Author

But without the redis oplog package, the latency compensation is working fine:
image
you can try it in the without-redisoplog branch

@maxnowack
Copy link
Contributor Author

Ah, the root of the issue isn't the result of the method, but the updated message for the method

@theodorDiaconu
Copy link
Contributor

@maxnowack you are correct I am now investigating. this is still not good.

@theodorDiaconu
Copy link
Contributor

I'm absolutely baffled, I have no idea why client-side inserts work from my other application without your package..

@maxnowack
Copy link
Contributor Author

That's the piece of code, where the updated message is being send: https://github.com/meteor/meteor/blob/devel/packages/ddp-server/livedata_server.js#L643-L656

@theodorDiaconu
Copy link
Contributor

theodorDiaconu commented Nov 24, 2016

was just looking there, it seems that they somehow await other publications to do their thing before returning the result. It means that we need to somehow hook into that fence. I am now looking how their publication work, I think there lies the solution. (Now searching for usage of : DDPServer._CurrentWriteFence )

@theodorDiaconu
Copy link
Contributor

theodorDiaconu commented Nov 24, 2016

https://github.com/meteor/meteor/blob/devel/packages/mongo/mongo_driver.js#L295-L309

Made progress on this managed to get the change from a client-side insert inside the ObservableCollection now I need to understand how to make it performant, because I'm only getting the id and collection name.

@theodorDiaconu
Copy link
Contributor

So... here is the deal if you are curious:
https://github.com/meteor/meteor/blob/devel/packages/mongo/mongo_driver.js#L363-L366

// this is the refresh method.
Meteor.refresh = function (notification) {                                                                           // 8
  DDPServer._InvalidationCrossbar.fire(notification);                                                                // 9
};     

So basically all listeners to the _InvalidationCrossbar are fired, before the end-result of a method is pushed to the client. So they can do it in sync and do the updates, before sending that 'update' message we saw.

// in RedisSubscriber we could do something like this.
DDPServer._InvalidationCrossbar.listen({ collection: observableCollection.collectionName}, (notification) => {
            const { id } = notification;
            // process the query
            console.log(notification); // looks like { collection: 'X', id: 'Y' }
        })

Oh boy what have I gotten myself into...

@maxnowack
Copy link
Contributor Author

@theodorDiaconu this seems to be broken again in 1.1.6 :(

@theodorDiaconu
Copy link
Contributor

@maxnowack hmm, I did not actually test, but I kept the compensateLatency from 1.1.5. Will check. Should I look here https://github.com/maxnowack/meteor-redisoplog-latencycompensation ?

@maxnowack
Copy link
Contributor Author

@theodorDiaconu yes, I've just updated the repo to the latest version of redis-oplog

@maxnowack
Copy link
Contributor Author

@theodorDiaconu please reopen this issue. I'll try to write some tests for this case

@theodorDiaconu
Copy link
Contributor

it makes sense yes. it was an easy fix, but I have no idea how to test this.

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

No branches or pull requests

2 participants