search

appfeel / node-pushnotifications

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

Pump version of node-apn to 3.x in order to remove the memory leak #70

Closed bubuzzz closed 4 years ago

bubuzzz commented 6 years ago

Currently, node-apn is in version 2.2 thus, it lead to this issues https://github.com/node-apn/node-apn/issues/518. Please pump the version up so we can resolve it. Thanks

alex-friedl commented 6 years ago

Same issue as https://github.com/appfeel/node-pushnotifications/issues/44.

@bubuzzz Updating to node-apn 3.x will result in the library only being compatible with node 8.8.x+. Hence this should be considered rather carefully. Are there other known fixes for node-apn 2.x ? Like https://github.com/appfeel/node-pushnotifications/pull/55 ?

jpike88 commented 6 years ago

Just a reminder that I have a fork of this repo running with node-apn-http2 builtin... running with zero issues. Considering this issue is still open and people are trying to modernize, feel free to use this instead. And of course, it requires node 8.8.1+.

@appfeel appears to be off the grid right now, so if anyone wants an alternative that is proven to work in production:

npm install node-pushnotifications-http2

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

samsprakash commented 6 years ago

@jpike88 : I am using Lambda + node v8.10. I used node-pushnotifications-http2. However I am facing the same issue. Is it working for you?

jpike88 commented 6 years ago

That fork I linked works with no issues.

miqmago commented 6 years ago

@jpike88 is it possible to adapt modifications to this plugin? could you point us where is the memory leak? Thanks!

jpike88 commented 6 years ago

I still don't know exactly what the nature of the memory leak is, all I know is that the native version of http2 seems to have fixed it

amangeot commented 6 years ago

Hello good people, I ran into similar issues, memory leaks with event emitters preventing my script from exiting, hence leading to issues when running the script multiple times

I was wondering if you had issues when running node-apn with a nodejs server that keeps running? Or does it happen when closing only?

kermopajula commented 5 years ago

Any updates on this? The memory leaks are still present.

jpike88 commented 5 years ago

If you want a quick fix use this

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

jamesholcomb commented 5 years ago

@jpike88 Do you plan on catching up your fork and publishing?

jpike88 commented 5 years ago

Nope, works like a charm in heavy production environments. ‘if it ain’t broke’...

If there’s a big incentive for me to upgrade it I’ll take a look but it handles iOS and Android notifications fine

jamesholcomb commented 5 years ago

There have been many bug fixes and issues addressed since May-6 2017 (the date of the last merged commit in the -http2 fork). In particular for me, #58.

miqmago commented 5 years ago

Possible solutions:

Waiting for node-apn/node-apn#518 to be solved... Right now it seems there is no interest on solving it.

vitalyster commented 5 years ago

@jpike88 can you please open issues for your fork?

jpike88 commented 5 years ago

Done.

@miqmago The fork just uses the native http2 library bundled with node 8.8.1+, which seems to have eliminated the memory leak warning. Not sure how it works either, it was initially just an experiment but it's been on production for quite a while now, no issues.

alex-friedl commented 4 years ago

With version 1.2.0 we migrated to https://www.npmjs.com/package/@parse/node-apn which is an actively maintained fork of node-apn. Does the memory leak still persist with the new release?

jpike88 commented 4 years ago

good question! I'll move back to main branch now and see how it flies

alex-friedl commented 4 years ago

@jpike88 any update on this?

jpike88 commented 4 years ago

Sorry, I've been too busy, I got part way in and then some business shit hit the fan and I haven't touched it since.

Feel free to give it a go... bear in mind that my fork only touched the library's code very lightly, I probably don't have any deeper understanding of the code than you.

derwaldgeist commented 4 years ago

Has this already been resolved? I'm a bit confused because package.json does in fact reference node-apn version 3.1.0.

alex-friedl commented 4 years ago

@derwaldgeist We upgraded to a fork of node-apn that should be maintained more actively than the original one. I assume the issue is resolved until we hear otherwise :)