MatthieuLemoine / push-receiver

A library to subscribe to GCM/FCM and receive notifications within a node process.
https://medium.com/@MatthieuLemoine/my-journey-to-bring-web-push-support-to-node-and-electron-ce70eea1c0b0
MIT License
198 stars 75 forks source link

High CPU consumption when internet connection is lost #2

Closed PedroKantar closed 6 years ago

PedroKantar commented 6 years ago

Matthieu,

When the internet connection is lost, in client/socket/index.js , connectSocket() starts looping on retry(), which results in a nasty CPU overhead.

To prevent it, I modified connectSocket() prototype, by adding a "retry tempo" parameter, so that updated code in index.js looks like that:

What do you think? I tested it and it works quite well. I set the tempo to 15sec, even if it could be set at interface level by adding a new parameter to function connect().

Regards, Pedro.

MatthieuLemoine commented 6 years ago

Maybe it would be better to do it with an increasing timeout depending on the number of retries with a maximum value to avoid a too large timeout :

const retryTimeout = Math.min(retries * 1000, 30 *1000); 

With this solution, we can reconnect instantly if the disconnection was just a temporary network failure instead of waiting for 15 seconds.

MatthieuLemoine commented 6 years ago

Feel free to submit a PR, if you have time to implement it !

PedroKantar commented 6 years ago

In V1.1.4 the retryCount is never reseted : The line retryCount = 0; // Reset retry count on successful in the socket.connect() connection has no effect, since the retryCount value will be taken from the last bind retry function :

// Retry on disconnect
  retry = connect.bind(
    this,
    payload,
    RequestType,
    preBuffer,
    NotificationSchema,
    keys,
    persistentIds,
    retryCount + 1
  );

I tried to find a "nice" solution based on current code (I would have submitted a PR), but I ended up with the solution I proposed in my first commit, here after: [FIX] High CPU consumption when internet connection is lost #2, where it was based on defining "let retryCount = 0;" outside "async function connect()" scope.. 🤔

PedroKantar commented 6 years ago

I'm going to submit a PR..