appfeel / node-pushnotifications

Push notifications for GCM, APNS, MPNS, AMZ (automatic detection from device token)
MIT License
534 stars 126 forks source link

Possible EventEmitter memory leak detected #44

Closed sbat closed 6 years ago

sbat commented 7 years ago

I get this warning:

(node:70543) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 wakeup listeners added. Use emitter.setMaxListeners() to increase limit

The warning comes from the APN Module: https://github.com/node-apn/node-apn/issues/518

See someone else this error? Have someone a solution?

alex-friedl commented 7 years ago

Hi @sbat, from the issue you referenced it looks like it's an open issue with node-apn?

dertieran commented 7 years ago

I think this happens because you always create a new connection and never close it. From the node-apn readme:

You should only create one Provider per-process for each certificate/key pair you have. You do not need to create a new Provider for each notification. If you are only sending notifications to one app then there is no need for more than one Provider.

If you are constantly creating Provider instances in your app, make sure to call Provider.shutdown() when you are done with each provider to release its resources and memory.

So something like this would help:

const connection = new apn.Provider(settings.apn);
return connection.send(message, regIds)
  .then((response) => {
    /* ... */
    connection.shutdown();
    return resumed;
  })
  .catch((error) => {
    connection.shutdown();
    throw error;
  })

But as mentioned in #47 it would be better to reuse the connection and offer a push.shutdown() or something like this.

heyjared commented 7 years ago

Is there a way to manually shutdown the connection to the provider?

flipace commented 6 years ago

When will #55 be merged and released?

jpike88 commented 6 years ago

Looks like the pull request doesn't really fix the issue apparently. Do yourself a favour everyone and update to Node 8.8.1+... I have released a package that eliminates the memory leak issue.

https://github.com/streaka/node-pushnotifications-http2

alex-friedl commented 6 years ago

Merging #55 as a quick fix for the memory leak issue. Will look into changing the implementation to only use one Provider instance, if feasible.

alex-friedl commented 6 years ago

Fixed after merge of #55