MatthieuLemoine / electron-push-receiver

A module to bring Web Push support to Electron allowing it to receive notifications from Firebase Cloud Messaging (FCM).
https://medium.com/@MatthieuLemoine/my-journey-to-bring-web-push-support-to-node-and-electron-ce70eea1c0b0
MIT License
191 stars 62 forks source link

All notifications are resent after each reconnection #4

Closed dupuydauby closed 6 years ago

dupuydauby commented 6 years ago

Hello,

It seems that after a deconnection and reconnection: Connection to MCS server lost MCS socket closed after 49 minutes Trying to reconnect MCS client connected

I receive again all notifications since the beginning of the app.

In your blog you said you should have fixed it I think? But for now, I just skip them if they have already been received.

Tested on last version 1.2.0

MatthieuLemoine commented 6 years ago

That's weird because electron-push-receiver saves all the notification ids that has been received to avoid this issue.

The ids are saved using electron-config. You can see it using :

const Config = require('electron-config');
const persistentIds = config.get('persistentIds') 

console.log(persistentIds);
dupuydauby commented 6 years ago

Thanks for the feedback.

I understand that we have a list of notification Ids saved in order to avoid receiving twice the same notification.

But is this list also given at the reconnection if there is a deconnection to the MCS server? Or are the Ids of the list changing when we reconnect with same credentials?

MatthieuLemoine commented 6 years ago

For now the list is not sent when reconnecting so the client will receive all/some notifications again but they are filtered client side. Already received notifications are discarded by the client.

// Already received
if (persistentIds.includes(object.persistentId)) {
  return;
}
PedroKantar commented 6 years ago

I reproduce this issue: after losing connection all the notifications are resent, looks like persistentIds is lost. A candidate fix in push-receiver\src\client\socket\index.js would be to reload ```persistentIds" before performing the retry :

persistentIds = config.get('persistentIds') || [];

I'm going to test it...

MatthieuLemoine commented 6 years ago

I think I know why you have this issue.

In retry, we are using the persistentIds passed on the first connection attempt. So if new notifications are received during a connection, we will receive them again on subsequent retries.

How to reproduce :

Can you confirm this @PedroKantar and @dupuydauby ?

MatthieuLemoine commented 6 years ago

But you shouldn't receive all the notifications. Only those which have been received since the first connection attempt. All the previous notifications from previous sessions should not be received again.

MatthieuLemoine commented 6 years ago

The solution is simple, we just have to update the persistentIds array when we receive a new notification.

Should be done in push-receiver

PedroKantar commented 6 years ago

"Can you confirm this @PedroKantar and @dupuydauby ?" Yes I do. This exactly the sequence I observed, only the "delta" notifications between the last connect and the retry are received.

PedroKantar commented 6 years ago

@MatthieuLemoine : what do you think in this line , as a first candidate fix?

persistentIds = config.get('persistentIds') || [],
MatthieuLemoine commented 6 years ago

Well we don't have access to electron in push-receiver as it should work also in Node without electron.

A better way will be to update the persistentIds array on each notification here


persistentIds.push(object.persistentId);
PedroKantar commented 6 years ago

OK, my previous fix worked, but I did not catch the "should work also in Node without electron" point, I'm going to test your candidate.. And get back asap.

PedroKantar commented 6 years ago

It works!

MatthieuLemoine commented 6 years ago

Great ! Can you submit a PR on push-receiver @PedroKantar ?

PedroKantar commented 6 years ago

Yes!

MatthieuLemoine commented 6 years ago

Fixed in v1.2.2

dupuydauby commented 6 years ago

Thanks guys, working very well!