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

Uniqush doesn't handle invalid payload received by apns server #24

Closed
cmabastar opened this Issue Jul 3, 2013 · 14 comments

Comments

Projects
None yet
2 participants
@cmabastar

cmabastar commented Jul 3, 2013

GCM is ok but apns has restrictions of maximum payload of 256 bytes. This happens when the user sends a huge message or simply messages that are utf8-encoded. Uniqush should complain about this issue.

Uniqush will be OOM killed by the kernel if handling this 'invalid payload'. We tested this with concurrent connections sending messages via celery. We had 8 workers and with 4 concurrency running each worker. When trying to send an invalid payload size, uniqush will crash and killed by the kernel.

@ghost ghost assigned monnand Jul 3, 2013

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 3, 2013

Member

@cmabastar-gumi Thanks for your report!

uniqush-push does not check the size of the message, I can fix this. However, OOM is not what I expected. Even if the message is too large, as long as it fits into the memory, there should not be OOM.

I tried UTF-8 characters with APNS, it can be correctly sent (and displayed on the screen.)

Let me see if I can reproduce the bug. It would be great if you can provide me more information so that I can easily reproduce it.

Member

monnand commented Jul 3, 2013

@cmabastar-gumi Thanks for your report!

uniqush-push does not check the size of the message, I can fix this. However, OOM is not what I expected. Even if the message is too large, as long as it fits into the memory, there should not be OOM.

I tried UTF-8 characters with APNS, it can be correctly sent (and displayed on the screen.)

Let me see if I can reproduce the bug. It would be great if you can provide me more information so that I can easily reproduce it.

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 3, 2013

Member

@cmabastar-gumi I tried large payload with version 1.4.3, it can report error correctly. However, I didn't try concurrent connections. BTW, I just added a line of code to check the payload size before sending it. I hope it can fix this problem. Do you want me to build a binary for you? (it's in the exp branch)

Member

monnand commented Jul 3, 2013

@cmabastar-gumi I tried large payload with version 1.4.3, it can report error correctly. However, I didn't try concurrent connections. BTW, I just added a line of code to check the payload size before sending it. I hope it can fix this problem. Do you want me to build a binary for you? (it's in the exp branch)

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 3, 2013

Member

@cmabastar-gumi I probably figured out the reason of OOM error. Would you please provide the answer for the following questions?

  • Are you using sandbox?
  • How many notifications have you sent before the crash?
  • Does uniqush crash even if you sent huge amount of valid notifications?
Member

monnand commented Jul 3, 2013

@cmabastar-gumi I probably figured out the reason of OOM error. Would you please provide the answer for the following questions?

  • Are you using sandbox?
  • How many notifications have you sent before the crash?
  • Does uniqush crash even if you sent huge amount of valid notifications?
@cmabastar

This comment has been minimized.

Show comment
Hide comment
@cmabastar

cmabastar Jul 4, 2013

@monnand Hi, Sorry for the late reply. Just got into the office.

  • Are you using sandbox?
    no we didn't use sandbox for this.
  • How many notifications have you sent before the crash?
    The number of notifications varied, as long as the invalid payload size were being sent continuously.

Here's the message from /var/log/messages: (Not sure if this helps )
Jul 3 12:25:42 ip-10-196-26-22 kernel: [7960078.943492] Out of memory: Kill process 15210 (uniqush-push) score 913 or sacrifice child
Jul 3 12:25:42 ip-10-196-26-22 kernel: [7960078.943501] Killed process 15210 (uniqush-push) total-vm:4286864kB, anon-rss:3620852kB, file-rss:0kB

  • Does uniqush crash even if you sent huge amount of valid notifications?
    NO, it seems to work nicely though sending through apns was still slow.
  • UTF-8 characters:
    yes, they are smoothly received by the device. The only gotcha we had was when trying to support japanese messages. Of course latin characters have different bytesize compared to utf-8 characters. So Instead of supporting 256 length of characters, we reduced the length of characters when sending japanese.
  • Overall
    This problem should be fix as long as the 'invalid payload size' is properly handled by uniqush.

cmabastar commented Jul 4, 2013

@monnand Hi, Sorry for the late reply. Just got into the office.

  • Are you using sandbox?
    no we didn't use sandbox for this.
  • How many notifications have you sent before the crash?
    The number of notifications varied, as long as the invalid payload size were being sent continuously.

Here's the message from /var/log/messages: (Not sure if this helps )
Jul 3 12:25:42 ip-10-196-26-22 kernel: [7960078.943492] Out of memory: Kill process 15210 (uniqush-push) score 913 or sacrifice child
Jul 3 12:25:42 ip-10-196-26-22 kernel: [7960078.943501] Killed process 15210 (uniqush-push) total-vm:4286864kB, anon-rss:3620852kB, file-rss:0kB

  • Does uniqush crash even if you sent huge amount of valid notifications?
    NO, it seems to work nicely though sending through apns was still slow.
  • UTF-8 characters:
    yes, they are smoothly received by the device. The only gotcha we had was when trying to support japanese messages. Of course latin characters have different bytesize compared to utf-8 characters. So Instead of supporting 256 length of characters, we reduced the length of characters when sending japanese.
  • Overall
    This problem should be fix as long as the 'invalid payload size' is properly handled by uniqush.
@cmabastar

This comment has been minimized.

Show comment
Hide comment
@cmabastar

cmabastar Jul 4, 2013

@monnand ,

How do you suggest to improve our apns push? Currently we only have 1 uniqush server that is handling 32 concurrent connections, it seems that whenever we are sending apns push it slows down on its http-requests. GCM is ok and fine.

cmabastar commented Jul 4, 2013

@monnand ,

How do you suggest to improve our apns push? Currently we only have 1 uniqush server that is handling 32 concurrent connections, it seems that whenever we are sending apns push it slows down on its http-requests. GCM is ok and fine.

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 4, 2013

Member

@cmabastar-gumi Your report is very helpful. I just tested the APNS and it is really slow.

Now let's first focus on the payload size. Right now, I have created a set of binary files which has already fixed the payload size problem. i.e. It will refuse to send anything if the payload is too large. Besides, it also fixed a memory leak bug, which may or may not be related to the OOM error.

These binary files can be downloaded from:

I'm not sure if they can fix your problem, but you can give it a try.

NOTE: I did not improve performance in the code now. I will profile the program and improve the performance in the next week. (I have a travel plan this week.)

Thank you very much for your report. I will do my best to fix it.

Member

monnand commented Jul 4, 2013

@cmabastar-gumi Your report is very helpful. I just tested the APNS and it is really slow.

Now let's first focus on the payload size. Right now, I have created a set of binary files which has already fixed the payload size problem. i.e. It will refuse to send anything if the payload is too large. Besides, it also fixed a memory leak bug, which may or may not be related to the OOM error.

These binary files can be downloaded from:

I'm not sure if they can fix your problem, but you can give it a try.

NOTE: I did not improve performance in the code now. I will profile the program and improve the performance in the next week. (I have a travel plan this week.)

Thank you very much for your report. I will do my best to fix it.

@cmabastar

This comment has been minimized.

Show comment
Hide comment
@cmabastar

cmabastar commented Jul 4, 2013

Thanks @monnand

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 4, 2013

Member

@cmabastar-gumi Now I'm pretty sure the performance issue is caused by the feedback checking mechanism. It is considered as a bug, see #26. I will upload a fix soon.

Member

monnand commented Jul 4, 2013

@cmabastar-gumi Now I'm pretty sure the performance issue is caused by the feedback checking mechanism. It is considered as a bug, see #26. I will upload a fix soon.

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 4, 2013

Member

@cmabastar-gumi Please try the newly created binaries. If you have any other problem, please let me know.

Member

monnand commented Jul 4, 2013

@cmabastar-gumi Please try the newly created binaries. If you have any other problem, please let me know.

@cmabastar

This comment has been minimized.

Show comment
Hide comment
@cmabastar

cmabastar Jul 5, 2013

Thanks for the quick release. This helps a lot :D

cmabastar commented Jul 5, 2013

Thanks for the quick release. This helps a lot :D

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 7, 2013

Member

@cmabastar-gumi Great! If it fixed your problem, then I would consider this issue, #25 and #26 as fixed. Please let me know if there is still problem.

Member

monnand commented Jul 7, 2013

@cmabastar-gumi Great! If it fixed your problem, then I would consider this issue, #25 and #26 as fixed. Please let me know if there is still problem.

@monnand monnand closed this Jul 7, 2013

@cmabastar

This comment has been minimized.

Show comment
Hide comment
@cmabastar

cmabastar Jul 7, 2013

Hi,

Just wanna know if you'll do a fix to improve sending messages to APNS? I'm
assuming in the near futrue?

Cheers,

Christopher Marlon S. Abastar

On Sun, Jul 7, 2013 at 3:23 PM, monnand notifications@github.com wrote:

@cmabastar-gumi https://github.com/cmabastar-gumi Great! If it fixed
your problem, then I would consider this issue, #25https://github.com/uniqush/uniqush-push/issues/25and
#26 #26 as fixed. Please
let me know if there is still problem.


Reply to this email directly or view it on GitHubhttps://github.com/uniqush/uniqush-push/issues/24#issuecomment-20567011
.

This email and any files transmitted with it are confidential and intended
solely for the use of the individual or entity to whom they are addressed.
If you have received this email in error, please notify the system manager.
This message contains confidential information and is intended only for the
individual named. If you are not the named addressee, you should not
disseminate, distribute or copy this email. Please notify the sender
immediately by email if you have received this email by mistake and delete
this email from your system. If you are not the intended recipient, you are
notified that disclosing, copying, distributing or taking any action in
reliance on the contents of this information is strictly prohibited.

cmabastar commented Jul 7, 2013

Hi,

Just wanna know if you'll do a fix to improve sending messages to APNS? I'm
assuming in the near futrue?

Cheers,

Christopher Marlon S. Abastar

On Sun, Jul 7, 2013 at 3:23 PM, monnand notifications@github.com wrote:

@cmabastar-gumi https://github.com/cmabastar-gumi Great! If it fixed
your problem, then I would consider this issue, #25https://github.com/uniqush/uniqush-push/issues/25and
#26 #26 as fixed. Please
let me know if there is still problem.


Reply to this email directly or view it on GitHubhttps://github.com/uniqush/uniqush-push/issues/24#issuecomment-20567011
.

This email and any files transmitted with it are confidential and intended
solely for the use of the individual or entity to whom they are addressed.
If you have received this email in error, please notify the system manager.
This message contains confidential information and is intended only for the
individual named. If you are not the named addressee, you should not
disseminate, distribute or copy this email. Please notify the sender
immediately by email if you have received this email by mistake and delete
this email from your system. If you are not the intended recipient, you are
notified that disclosing, copying, distributing or taking any action in
reliance on the contents of this information is strictly prohibited.

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 8, 2013

Member

@cmabastar-gumi How's the performance of the recently build? (version 1.4.4) If it is still a bottleneck, then it definitely deserves a high priority.

Member

monnand commented Jul 8, 2013

@cmabastar-gumi How's the performance of the recently build? (version 1.4.4) If it is still a bottleneck, then it definitely deserves a high priority.

@monnand monnand reopened this Jul 8, 2013

@monnand

This comment has been minimized.

Show comment
Hide comment
@monnand

monnand Jul 12, 2013

Member

I will consider this as fixed and put my effort on #27

Member

monnand commented Jul 12, 2013

I will consider this as fixed and put my effort on #27

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