deepstreamIO / deepstream.io-client-js

The Browser / Node.js Client for deepstream.io
Other
294 stars 109 forks source link

Record subscription after reconnect #598

Closed kristofkalocsai closed 5 months ago

kristofkalocsai commented 1 year ago

If the server drops out, and the client loses connection, after reconnecting (dsClient.getConnectionState() === "OPEN") record subscriptions are not fired.

To Reproduce Fork this sandbox to enable terminals and the restart option: https://codesandbox.io/p/sandbox/recordsubscribereconnect-bvxkr Check out the start log, where a server (with the producer client bundled inside) periodically writes a record. In a separate terminal, start the consumer client with node src/client.js. After the client successfully connects, you'll see it printing out the values the producer writes to the record. Now, restart the server. The client reconnects after a while, (prints RECONNECTING...OPEN to the log, which comes from the connectionStateChanged event), but does not print out the values anymore, therefore not getting notified about the record changes. On closer inspection, the client does emit UNSOLICITED_MESSAGE events, with topic RECORD on its error event.

Expected behavior Even after a restored connection, the clients should be notified via the subscribe callback.

Server version 6.2.2 (could not get codesandbox.io to work with node 18, but locally on my env. works just the same with 7.0.5)

Node version v16.17.0

Client (Js, Java, other) and client version 6.0.5 (could not get codesandbox.io to work with node 18, but locally on my env. it fails even with 7.0.1)

jaime-ez commented 1 year ago

Hi, I'm not familiar with codesandbox so I don't know about the details of the platform. I copy pasted the files locally on my computer and run server, then client, then stop and restart server: the client reconnects and continues to be susbscribed as expected and logs the updated values... can you check again? Wasn't able to reproduce the described bug...

kristofkalocsai commented 1 year ago

Hi! Oof, bad news. Did you prepare the whole project? server, client and dependencies? Checked again, of course I can reproduce locally, that's where my problems started :) image Node version: v18.15.0, Windows 10 "dependencies": { "@deepstream/client": "6.0.5", "@deepstream/server": "6.2.2" } Do you have any suggestions where/how can I create a more straightforward env. than the codesandbox?

jaime-ez commented 1 year ago

Ok, I can reproduce the bug only with "dependencies": { "@deepstream/client": "6.0.5", "@deepstream/server": "6.2.2" } when I update the client to v7 there is no bug... can you try again with "dependencies": { "@deepstream/client": "7.0.1", "@deepstream/server": "6.2.2" } ?

jaime-ez commented 1 year ago

I can confirm that client version 7.0.1 fixes the issue, however there are some considerations: what is actually happening is that when using a server without storage, restarting the server actually " deletes" all existing records. Therefore when the client reconnects it thinks that the record was deleted and thus the previous behaviour was to handle the conflict via the merge strategy, since the default merge strstegy is that remote wins, and it thought that remote was deleted, it assumed the record didn't exist anymore and therefore no resubscription was made. This was fixed in in v7 by setting the record as "dirty" when connection is lost. Let me know if you still have problems

kristofkalocsai commented 1 year ago

I can confirm, that for ordinary records, v7 solves this issue, thanks a bunch!

In our case, we'd like to use records with storageExclusionPrefixes, where the issue is still present (if and only if using storage!) Please see https://codesandbox.io/p/sandbox/recordsubscribereconnect-forked-jg63zb for the source.

Thank you for your continued support!

jaime-ez commented 1 year ago

The problem you're having is due to a race condition between which client logs in first to the server after the restart, and this is more evident when storage is enabled because the server takes longer to start. In your sample code:

const server = new Deepstream(options);
server.start();
// in the following line the client tries to conect inmediately to the server, but the server might not be ready yet,
// thus it triggers a timeout in order to try again in a few milliseconds, 
// and then the producer client might take longer to connect than the consumer depending on when the reconnection is 
// triggered
const dsClient = new DeepstreamClient("ws://127.0.0.1:6020");
await dsClient.login({ username: "producer" });

So if the consumer logs in first, the record won't actually exist yet and the default merge strategy will set it as such.

This is a fix for your sample code:

const server = new Deepstream(options);
server.start();
server.on('started', async () => {
    const dsClient = new DeepstreamClient("ws://localhost:6020");
    await dsClient.login({ username: "producer" });
    // the the rest of the logic...

}

that way the producer will log in inmediately when the server is ready.

And a last note, in your sample code you are a using an upsert method dsClient.record.setDataWithAck that doesn't require to get the record first. Lines 83 and 84 on your server.js file are not necessary when using that method. They are if you set the data as record.setWithAck() check here

kristofkalocsai commented 1 year ago

Yes, I indeed took some liberty with my examples, making the producer and the server the same entity. Take the following example: https://codesandbox.io/p/sandbox/recordsubscribereconnect-forked-rokwwk A server and two clients, one is a producer, the other one consumes a record's contents.

According to your response, in case of a server restart, I should somehow guarantee that the producer always reconnects first? Or implement in every client, that in case of a server drop, after connecting again, acquire new instances of the records which it was subscribed to, and resubscribe? Or am I in the dark completely? 😀

Please try:

  1. start server
  2. start consumer
  3. start producer
  4. stop producer
  5. stop & immediately restart server
  6. restart producer
  7. correct values, UNSOLICITED_MESSAGE or nothing at all in consumer
jaime-ez commented 1 year ago

The question is, which data you want to preserve? If it is a record that is not saved to storage, you want in case of a server restart to start with an empty record or keep the values the client has? If you wan't an empty record, you should check in the consumer if the record exists or not before resubscribing, or reset the record also on the client in case of a server drop and that way keep the subscription active. Or another option is to set the merge strategy to LOCAL_WINS for that record like this:

record.setMergeStrategy((localValue, localVersion, remoteValue, remoteVersion, callback) => { callback(null, localValue) })
jaime-ez commented 5 months ago

closing due to inactivity, please reopen if required