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

Very nicely done #2

Open
nevf opened this issue Jan 4, 2017 · 26 comments
Open

Very nicely done #2

nevf opened this issue Jan 4, 2017 · 26 comments

Comments

@nevf
Copy link

nevf commented Jan 4, 2017

@nponiros I'm using sync-server with a change to use Websockets instead of http. This is based on the Dexie.js Websocket samples.

So far I've only had to replace your server.js with mine and much to my amazement it worked first time. You've done a great job of restructuring the Dexie sample code.

I am having a problem with specific changes not syncing when there are multiple clients and don't know yet whether this is a problem in my websocket server implementation or elsewhere. Have you tried sync-server with multiple clients (Browsers). ie. Add data in one and check it syncs correctly in the other one?

Also could you look at sendAnyChanges() in WebSocketSyncServer.js and let me know if your getServerChanges() should match the behaviour of sendAnyChanges(). ref this comment:

Get all changes after syncedRevision that was not performed by the client we're talkin' to.

The XHR implementation does work a bit differently to the Websocket one which starts with a "subscribe" action to get any changes from the server that aren't in the client. This may be where I'm going wrong.

I'm calling syncHandler() when the subscribe message is received with syncData including the syncedRevision.

@nponiros
Copy link
Owner

nponiros commented Jan 4, 2017

No I haven't tested with multiple browsers yet. My first suspicion would be that you are missing the clientIdentity (or that the server is doing something stupid with it). You can check the changes.table file saved on you disk to see if the changes have a 'source' property. Also check the meta.json file to see if it includes the client identities with their revisions.

You would probably want to split up the sync/handler.js into two files one to react on client changes and one to send changes to the client when the server receives a change from another client. So basically you would use the applyClientChanges() part for when the client sends data and you would use the getServerChanges() part as the callback function during "subscribe". This is not the most efficient way but the easiest currently.

Yes sendAnyChanges() and getServerChanges() are pretty much the same. The main differences are that getServerChanges() has support for partials and it uses promises.

So I hope this helps you a little bit.

PS: I'm not sure I the socket server example even supports multiple client connections at once. It seems to only have one connection object but I'm not familiar with websockets.

@nevf
Copy link
Author

nevf commented Jan 4, 2017

@nponiros The changes.table includes source=clientIdentity and meta.json looks reasonable.

changes.table

{"rev":51,"source":1,"type":1,"table":"friends","key":"cixijdwcy0000385nwhfn0r13","obj":{"id":"cixijdwcy0000385nwhfn0r13","name":"Arne","shoeSize":47},"_id":"aiw7nMN6CwjPghkr"}
...

meta.json

{"revision":63,"nextClientID":1,"clients":{"1":{"revision":51},"2":{"revision":54},"3":{"revision":57},"4":{"revision":60},"5":{"revision":63}},"tables":["friends"]}

Are you planning on testing multiple browsers anytime sooner. If so I'd rather leave the changes you suggest to you.

@nponiros
Copy link
Owner

nponiros commented Jan 4, 2017

Hmm nextClientID should be 6 and not 1 nevertheless you did get the correct IDs for the clients assuming that you did use 5 different browsers. I will try to test tonight with multiple browsers (it's morning here now) and let you know.

@nevf
Copy link
Author

nevf commented Jan 4, 2017

FYI I'm just using two PC's but my little test app starts by deleting the Dexie database and then adds two documents. So I open the app in each PC/Browser and then refresh one, which deletes it's Dexie DB and then adds two doc's. Thanks for looking at this.

@nponiros
Copy link
Owner

nponiros commented Jan 4, 2017

@nevf So I did a quick test with 2 different browsers. Everything seems to be working fine. At least the server communication passed the data that I expected, the changes.table is correct and the nextClientID in the meta.json updates as expected. I had some issues with duplicated keys but that is probably an issue with my test data and/or my key generator on the client. After resetting everything (cleaning the server tables and IndexedDB in the browser) it worked without issues.

@nevf
Copy link
Author

nevf commented Jan 4, 2017

@nponiros Thanks for trying, good to hear it works correctly. I'll continue working on my code and hopefully resolve my issue.

@nponiros
Copy link
Owner

nponiros commented Jan 4, 2017

Well I do need to write integration tests and more unit tests to make sure everything works correctly and reliably but it looks okay for now. But I can tell you that the socket example from Dexie is not well tested so there might be a bug or two there. If you don't manage to get it to work you can send me your code and I can have a look on Saturday. You can also create a PR if you want so that the sync-server has a sockets implementation.

@nevf
Copy link
Author

nevf commented Jan 4, 2017

@nponiros When I use the WebSocketServer.js code it works correctly, but not when I use my code + your code. And I don't yet know where the issue lie. I've got limited time the next 2 days.

Adding websockets support to sync-server would be great. I'll create a PR.

I'm curious why you decided to use XHR and polling instead of sockets?

@nponiros
Copy link
Owner

nponiros commented Jan 4, 2017

Yes I used the logic from the socket server example but the function call order is differently and this might cause the issues you are seeing.

I use polling because most of the time the server is offline and my app disconnects from the server so sockets would be useless. I use the sync-server locally on my laptop and not on a real server.

@nevf
Copy link
Author

nevf commented Jan 5, 2017

@nponiros The WebSocketSyncProtocol.js code tries to reconnect to the server every 5 seconds, forever, so this should work fine with your frequently offline server.

I haven't made any further progress today, so if you can implement websockets in sync-server that would be great. I can send you my ws_server.js code, but you are probably better starting from scratch.

@nponiros
Copy link
Owner

nponiros commented Jan 5, 2017

@nevf just send me your code so I can get a feeling on how you are planing to use it before I implement something that might work completely different than what you expect/want. I can't give any guarantees on when I will be done with it but I will try to at least have a first version by Sunday.

@nevf
Copy link
Author

nevf commented Jan 6, 2017

@nponiros Thanks for your help. I'm not in any great hurry, so please take your time. I've attached a zip file with my ws_server.js and a slightly modified start.js of yours.

You will see various comments and TODO items.

I think the main issue is the xhr code relies on each client requesting server changes, whereas the websocket implementation pushes changes out to all connected clients whenever they occur. I think I have that part working ok.

However the websocket "subscribe" action isn't behaving as per the original sendAnyChanges() code.

A final comment. I feel that the data in meta.json needs to be in the database which means db.getNextClientID() needs to be async and return a promise.

Any questions please let me know.

ws_server.zip

@nponiros
Copy link
Owner

nponiros commented Jan 6, 2017

@nevf thanks for the code. I will have a look ASAP. Yes you are right that getNextClientID() and also other methods interacting with meta.json should be async but sync is good enough for me currently and will get to that when I have time. I will open an issue for it.

@nponiros
Copy link
Owner

nponiros commented Jan 7, 2017

@nevf so I add a WebSocket server. It's not really tested yet but it might help you with your sync issues. I will try to test it later today. The implementation is in the socket_server branch.

Edit: I made a small test using the socket sample I added to the repo. Things seem to work when I use 2 different browsers. I didn't test it with the socket sample from Dexie so it might not work with that yet.

@nevf
Copy link
Author

nevf commented Jan 8, 2017

@nponiros Thanks for that. Your socket sample works fine on two PCs, however my sample app with Dexie.syncable still isn't.

When I start the app on the second PC with an empty Dexie database, it isn't getting the existing changes from the server. New changes on either PC are synced correctly.

Edit 1: I'll check my app code matches your sample, which may fix this.
Edit 2: I suspect your code in index.js does not match all the functionality required by Dexie and used in WebSocketSyncProtocol.js which is why my app isn't working. You need to use WebSocketSyncProtocol.js in your sample and test that.

@nponiros
Copy link
Owner

nponiros commented Jan 8, 2017

@nevf make sure that you get a client id before sending changes and subscribing. The socket protocol in the dexie samples assumes a synchronous client id generation (this is needed in case you are using the master branch. The socket server branch creates the id synchronously so it should work with the sample code).

My samples get database changes but don't use syncable so I would say the server works. At least if the revision numbers are correct. If you want you can send me your client code and I will have a look or are you just using the socket sample from dexie without any other code?

@nevf
Copy link
Author

nevf commented Jan 8, 2017

@nponiros WebSocketSyncProtocol.js requests a clientIdentity from the server, as does your sample. I'll check that this is received before subscribing etc. and get back to you.

@nevf
Copy link
Author

nevf commented Jan 8, 2017

@nponiros Ok from a quick test the client code WebSocketSyncProtocol.js gets the "clientIdentity" message and then gets the "ack" message which process the subscribe. The subscribe request is sent straight after the clientidentity request in WebSocketSyncProtocol.js

            // When WebSocket opens, send our changes to the server.
            ws.onopen = function (event) {
                // Initiate this socket connection by sending our clientIdentity. If we dont have a clientIdentity yet,
                // server will call back with a new client identity that we should use in future WebSocket connections.
                ws.send(JSON.stringify({
                    type: "clientIdentity",
                    clientIdentity: context.clientIdentity || null
                }));

                // Send our changes:
                sendChanges(changes, baseRevision, partial, onChangesAccepted);

                // Get any server db changes since we last connected. And Subscribe to future server changes:
                ws.send(JSON.stringify({
                    type: "subscribe",
                    // changes: [],
                    requestId: requestId,
                    syncedRevision: syncedRevision
                }));
            }

@nponiros
Copy link
Owner

nponiros commented Jan 8, 2017

Are you using the master branch? In master this would not work. In the socket server branch it should work. For master you will have to wait for the response with type 'clientIdenty' before sending changes and subscribe. If you don't wait the server might not have enough time to associate the connection with the id. I will see if I find time tomorrow evening to test with the dexie sample.

Yes my sample code does not use revisions because it doesn't save any data this shouldn't be an issue unless I have other bugs in thd codd. Btw the master branch does have some revision related fixes which might solve your issue.

I will test with the dexie sample ASAP.

@nevf
Copy link
Author

nevf commented Jan 8, 2017

@nponiros I am definitely using the socket_server branch. I didn't npm install it, but downloaded the zip file instead and then run it from node.js directly after installing all the node modules.

Neither the WebSocketSyncProtocol.js code above nor your index.js wait for the clientidentity "ack" before sending the "subscribe" request.

I've attached a copy of my app-wss.js code I'm having the issue with, along with the Dexie files I'm using.

app-wss.zip

Thanks again for your efforts.

@nponiros
Copy link
Owner

nponiros commented Jan 9, 2017

@nevf so I found the mistake. I was using default params for syncedRevision and baseRevision but those don't work since during the initial sync Dexie.Syncable sends null and not undefined. I fixed that in the master branch so please use that. In order for everything to work you will have to move some code around in WebSocketSyncProtocol.js.

// Send our changes:
sendChanges(changes, baseRevision, partial, onChangesAccepted);

// Subscribe to server changes:
ws.send(JSON.stringify({
  type: "subscribe",
  syncedRevision: syncedRevision
}));

The above lines must be removed from the ws.open function and put into:

} else if (requestFromServer.type == "clientIdentity") {

This is due to the async generation of the clientIdentity. If the code remains as is you might be trying to subscribe before a clientIdentity was generated which means that the subscription can not be associated with the client ID (at least it can't be associated currently without adding some queue to wait for generation before subscribing). Now I hope you don't find any more bugs ;)

PS: in the master branch the interaction with meta.json is also async.

@nevf
Copy link
Author

nevf commented Jan 9, 2017

@nponiros Great and thanks. I'll get the updated master branch, make the change to WebSocketSyncProtocol.js and let you know how I go.

@nevf
Copy link
Author

nevf commented Jan 9, 2017

@nponiros Ok looks great. The problem I was having where the 2nd PC wasn't getting changes on subscribe is fixed. Many thanks.

I assume you've seen the console error: error: 5 'Error' 'You can\'t write to a non-open connection' when a Browser Refresh is done. I'm guessing the client that disconnected has not been removed from the code you use to track clients. This is with my test app.

Edit: This error only happens when a I delete the Dexie DB and therefore a new client id is created. So if I comment out: Dexie.delete( DB_NAME ); there is no error.

@nponiros
Copy link
Owner

nponiros commented Jan 9, 2017

@nevf great to hear that.

I'm actually handling close in socket/server.js line 46 but I forgot to pass the connection id..
Fixed it now and found out that I can edit files in the browser :)

The server needs some end to end tests. I'm not sure if the subscriptions should also be removed on error. My understanding is that error gets called if there was a transmition issue. I guess the client could try again although the dexie socket sample does stop syncing so removing the subscription might be the right thing to do. Do you have any insight on that?

@nevf
Copy link
Author

nevf commented Jan 9, 2017

@nponiros You shouldn't do anything in on.error other than log the error as you are doing. on.close should definitely remove the subscription. In this scenario the client has to open a new connection and hence a new subscription.

@nponiros
Copy link
Owner

nponiros commented Jan 9, 2017

@nevf okay, thanks for the input

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

No branches or pull requests

2 participants