New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update core.rb #22

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@theosp
Copy link

theosp commented Aug 27, 2013

Isn't this loop redundant and make the bulk of notifications to be sent more than once?!

Update core.rb
Isn't this loop redundant and make the bulk of notifications to be sent more than once?!
@wlipa

This comment has been minimized.

Copy link

wlipa commented Aug 28, 2013

We just experienced some serious multi-notification spam via the new library, so I think it may well be...

@wlipa

This comment has been minimized.

Copy link

wlipa commented Aug 28, 2013

Originally from the @toto checkin I think.

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Aug 28, 2013

@wlipa indeed, that is a bug I introduced when updating the lib. Will have a look why the tests did not catch this, they should have.

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Aug 28, 2013

I verified that this is indeed the cause of the multi-notification problem. @theosp can you please bump the bundle version as well (toto@f134e90), so @jpoz can quickly merge this in.

Building a regression test for this needs a bit more time, but it should be fixed ASAP.

@wlipa

This comment has been minimized.

Copy link

wlipa commented Aug 29, 2013

The library is very dangerous to use as is. 1.1 should be pulled unless a fix is forthcoming.

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Aug 30, 2013

True, unfortunately only @jpoz can do that. As he does not seem to notice this thread, I will try him via mail.

@theosp

This comment has been minimized.

Copy link

theosp commented Aug 30, 2013

@toto , @wlipa , @toto In my opinion 1.0 should be set as the production version of the gem for at least a couple of weeks. 1.1 needs more testing even with this fix.

@wlipa

This comment has been minimized.

Copy link

wlipa commented Sep 10, 2013

I wonder if we can get the gem pulled some other way if @jpoz is out of action. It really shouldn't be distributed in the current state as it very easily leads to an absolute nightmare of multiple alerts and very annoyed users.

@jpoz

This comment has been minimized.

Copy link
Owner

jpoz commented Sep 11, 2013

bad time for me to take a vacation i guess. I'll get this merged in. This maybe a a good time to have a collaborator on this gem. Any takers? @toto @theosp

@jpoz

This comment has been minimized.

Copy link
Owner

jpoz commented Sep 11, 2013

yanked 1.1.0

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Sep 12, 2013

Thanks for merging. would be glad to help.

I will make sure to build a regression test for this problem. Currently only the single notification is tested, not what get's written to the socket in the end. That is how this slipped through.

@mkonecny

This comment has been minimized.

Copy link

mkonecny commented Oct 4, 2014

Can this be pushed out, or is there still work to do?

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Oct 5, 2014

I have cleaned up a few things and added patches. Take a look at https://github.com/toto/APNS. I will do a new PR, so @jpoz can take a look. I bumped the version to 1.1.1 though, to ensure that everything get's updated correctly after the 1.1.0 debacle.

@toto

This comment has been minimized.

Copy link
Contributor

toto commented Oct 5, 2014

Also I should note that I use my 1.1.1 code in production sending out push to >100k devices - including content_availiblepushes. So the format implementation is robust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment