search

h2soft / node-gcm

MIT License
68 stars 20 forks source link

Multiple notifications in parrallel #7

Open alexislg2 opened 9 years ago

alexislg2 commented 9 years ago

Hey folks, I have a strange behaviour. Sometimes I send multiple notifications in parrallel. It means that I call gcm.send() multiple times. I know I could use an array of reg ID instead but it messages are not the same.

Everything works fine except when there is one error in one of the notifications. For example when one of the recepient has an expired reg Id, I receive the NotRegistered error. My problem is that I get this error for all the other notifications sent at the same time whereas there should be no error for the other recipients. This is strange

mamaral commented 8 years ago

+1

mamaral commented 8 years ago

I'm not a node js guy, but it appears to me this is probably due to there being n listeners created when you send n requests in a loop, and if one hits the error case it emits the sent event with that error and ALL of those n listeners are listening for it, so they ALL handle the error event.

That explains why if you send n notifications and 1 fails, you get n callbacks with that error.

Neamar commented 6 years ago

Yeah, when you look at the code it uses if (cb) this.once('sent', cb);, so if you send multiple notifications in parallel using the same object it will only notify one cb :\

Neamar commented 6 years ago

Fix: create a new GCM object everytime you need to send something in parallel. Alternative fix (require changes in the library): edit code to remove calls to self.emit('sent', ...) by direct calls to cb().

@h2soft I can send a PR, will you merge it and publish to npm?