search

j3k0 / ganomede-notifications

Long-pull notification service for Ganomede
0 stars 0 forks source link

Notifications sent many times to the same user #25

Closed j3k0 closed 8 years ago

j3k0 commented 8 years ago

On my android device, a user received 5 times the same notifications (every of them, not just sometimes).

@elmigranto can you look into that / work to a solution?

j3k0 commented 8 years ago

On redis:

127.0.0.1:6379> smembers notifications:push-tokens:jeko2:triominos/v1
1) "gcm:APA91bHoInN-6yLC9gDaaLpL3_7UN6mc0flpvygXgitjmmrHVlEl10fpDXeCsnY07V2d4NlN8eT0HWlEsxmj-Fo9YH4UGBetYx_ZYJV1yluEjZ1k40qvqtS-LIyAMSSPnZUtR-q8qTSV"
2) "apn:<4eda2064 af61c856 a6ff2abb 8075207e 046f739a f30b8a73 2c763aad 8c016af6>"
3) "gcm:APA91bHZkcWNWtKMSzwB0waAjm5hjpU0mB6WSGRaWSlEpbAn-CqELa62eFO9R5PHwNjm8wvglygX8lDR2sRV-2bkZ4leGpYYTgMQxBs2eXDoCvxuz_FpkUTcRZ_ilcBTMft1gTS20wOq"
4) "gcm:APA91bHvqqYalHbD2ANM9ZzQWQZQOfD1QKTj2UyLoa5FaPz0UtQ5N_nbJf3zszsiCqt70u_Ljyma1EE0WpJ7guBqiHJXI7m4m7Ee7IV0ee1tJuYvqx0Kz3dfyRGJznx8fmCO5_-nrQT1"
5) "gcm:APA91bFndw-YqLgWQY5_rPKcGYsgNhiOmR7wanZuF-qSLKAYycy0--IJNCKzdgd5LzLGacNEVMzb_XDr9CbDBM-xAgFHLQYTcVMigXcSEIjq_IkezM7RIGVe20PAFvlnB9j_osjkoluZ"
6) "apn:<13c3b8d1 9e484e22 ed412b62 9d7c5a8f a3c9595d 2619f55e ec064c53 ec72feac>"
7) "gcm:APA91bH1kGV2m9-C41JoKQ9RKWJG5Dy6BddsnHxj2tLpkeb2RbzACyaCLGT6hBa39mZx7KbOzUhQlKbxczP608NOKER7PcoXzfv0d22OcqWO1Ru8dwfOx2XpaWSlH_PEEG7iEW6L6gze"
8) "gcm:APA91bEY63Pyyr5ywVWiCpeoY5nnlVsg1iHWGUAN4VSLU4k4GdnNCl_GBwxp5lecRtN2Brc2vioZ52z96KX32BUC93nV5PfnMhCc7Mwxx1ehQMH1PvrJ8x1LvDYUo93UAcv5SVOPsnq_"
9) "gcm:APA91bH-huwBJk6KyH-cqzCFhktqK-2pvk9CH817bGIa5sc-UoLpJx83KrK3r4vCuBELIs7qFqEjeNhmsdEfXWbrNv3Sl4g06KwE730G7w1BzOANt3oC2XdYvNpmFPL3wzHCaB6blzXk"
j3k0 commented 8 years ago

I believe we shouldn't allow more than 1 "apn" and 1 "gcm" token for a given user...

Any change should be backward compatible: we can't break the existing database.

j3k0 commented 8 years ago

Proposed solution:

  1. write a separate "cleanup" script, which will run through all keys, keep only 1 gcm and 1 apn for each user.
  2. in push api, make sure newly added tokens override the previous one of the same type.
elmigranto commented 8 years ago

Is it possible that the user has multiple android tokens linked to his username?

Yes, for sure. I was thinking this is desired — someone might be using phone and tablet, receiving on both devices (same for apple).

Is it possible that the server sends multiple times the same notification?

Don't think so, it is more likely to lose some (pop'd from redis, failed to be sent for some reason). I see you have 7 gcm tokens saved in redis, is this from 1 device?

I believe we shouldn't allow more than 1 "apn" and 1 "gcm" token for a given user...

What if someone plays on multiple devices? This will mean they'll receive notifications on one device but not the other. If that's okay, this is the most trivial thing to implement.

I don't really have much experience here (only use 1 device, haven't build any mobile apps). What is appropriate behavior? What are technicalities of the process?

  1. User uses 2 devices — tablet at home, phone otherwise (both are google). Someone sends him a message, do we push notifications to both devices or only recently used one?
    1. Do we want notification to be sent to both devices?
    2. When user reads it, do we want to hide it from other device (probably not, also not really relevant here)?
  2. Do we receive push token every time user opens an app?
    1. If yes, wouldn't they be the same per app/device pair? Even if they aren't, wouldn't issuing new token for the same app/device invalidate previous ones? Can we distinguish between devices, should we?

I'll try to read up on those in the meantime, but would really appreciate you sharing your expertise here, since you have a lot more experience with this kind of thing.

elmigranto commented 8 years ago

On a sidenote, TokenStorage and redis data looks a bit odd:

  1. spop removes random member, should probably change this (https://github.com/j3k0/ganomede-notifications/commit/2b012d855e8ab158a5d86a0cf64544b61691622a)
  2. looks like apn tokens are buffers for some reason (Buffert.prototype.toString()), are those from tests or actual data? Looks like apple provides 32 bytes in binary format, not sure how iOS app handles it in terms of sending to push-api. node's apn module handles hex strings, maybe it sanitises them (removes <, > and whitespace). Thought this worth mentioning.
elmigranto commented 8 years ago

Looks like both, GCM and APN tokens are per app/device pair, can change in some cases invalidating previous one. Not sure there is a way to differentiate between devices of the same user, except if client application sends that information.

So we are probably sticking to one token per user per app per devices type for now?

If we receive token every time app launches, we can wipe redis DB, and move from set to hash with keys being device type.

If that's not the case, which token do we keep? There is no way to select "latest one" from redis set, so, random of that type?

elmigranto commented 8 years ago

On a second sidenote, have you looked into using GCM for iOS notifications? Would save some hassle, given Apple's way of sending pushes (i.e. #16).

j3k0 commented 8 years ago

No I didn't look, for one good reason, the client app uses the Adobe AIR SDK, for which the most popular Push Notifications plugin (the one we use) doesn't support GCM for iOS.

Anyway #16 (not handling invalid tokens) is really a minor issue.

There would ways to identify the device, for instance sending the model of the phone would be enough I guess (people rarely own and use 2 exactly identical phones). So, eventually, Iet's add an optional device field to the token registration request (default to something like "unknown").

I'd rather not wipe the DB. One way of handling it would be to use something like hset push/v2/gcm/username device value to add a new token, hgetall push/v2/gcm/username to retrieve them. When hgetall returns null, then look into the legacy redis field, and use a random entry from the hash.

Edit: no need for a cleanup script then

elmigranto commented 8 years ago

Edit: no need for a cleanup script then

Probably worth doing it, since we currently have tokens keyed at notifications:push-tokens:${username}:${app} with no version info, or do we bring in versioning?

We could also switch to other Redis db (say 1 instead of default 0). Or move old data to other db, convert to a new format in db 0, leave it for some time in case of any wrongdoing.

I was thinking of leaving redis keys the same (prefix:username:app), and hash fields would be ${type}:${deviceID} (i.e. apn:iphone6.1, gcm:nexus5), with current one being apn:default_device.