-
Notifications
You must be signed in to change notification settings - Fork 216
Use master branch of chatterbox #202
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
Conversation
Chatterbox in version 0.5.0 has a memory leak which has been there since at least December 2016. The problem is that finished HTTP/2 streams are not cleared out properly when you get their response. This means the Chatterbox process will keep allocating more and more memory as time goes on and eventually crash the Erlang node. Use the git-version of Chatterbox for now, which has the fix in place. This avoids memleaks of this kind.
There is an alternative implementation: Close the HTTP/2 connection every 1000 requests or so, but I'd much rather fix the underlying problem in chatterbox :) |
Great, I didn't know that. Could you please open a issue in order to track it? we need to use a hex chatterbox version as soon as they release a new one in order to deploy in hex. |
@@ -23,7 +23,7 @@ | |||
%% == Dependencies == | |||
|
|||
{deps, [ | |||
{chatterbox, "0.5.0"}, | |||
{chatterbox, {git, "https://github.com/joedevivo/chatterbox.git", {branch, "master"}}}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use something like {ref, "f5b68963bc63b3b2cf8701fafe61cfae9c0a383f"}
here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if we want to stay there. Usually I let the .lock file track these. But I can make this change if needed :)
@ferigis Open an issue on this repository or on chatterbox? I think if we ask nicely, they will probably cut a release for us. |
I meant create an issue here if we merge this PR but if we ask them too would be perfect! |
I think the right way is to get a new version and use that. If they, for some very odd reason, don't want to make a release, we can merge this PR. I'm just "throwing stuff over the fence" for things I needed to fix in my end to make things work in order to get the right, preferred solutions out there and push the ball in the right direction. I.e., the PR matter far less than the solution. But when you have code to go with an Issue, it is my experience things will be far more productive as there is a concrete solution one can discuss. |
Hi, I'll run the discussion in this PR, although it also pertains to the two other PRs #201 & #203 Ultimately, I rolled my own APNs push solution based on a number of observations:
I enabled this in production last Tuesday, and it seems to be working fairly well. So I'm not going to need this PR, as well as the ones above. Feel free to close them if you want, or leave them for later discussion if need be. The code we have might end up as Open Source at some point, but it isn't that much of a priority right now. It'll need some cleanup and documentation anyway :) In any case, I couldn't have written this so quickly were it not for the work you've made in apns4erl. Especially the openssl calls around the ES256 algorithm for JWT was a godsend. |
@jlouis I also had the memory leak issue and I also blamed chatterbox. There were heap binaries all over the world, but I didn't find the root cause. I chose a brute-force solution, I reconnect in every 15 mins. @ferigis I saw that chatterbox has version 'v0.7.0', it would be a good idea to bump the version, at least on a branch. @jlouis Do you plan to outsource your project? Is it for the public, is it configurable or something specific in one of your project? |
The root cause is that once a stream is done on the connection it has to be garbage collected by the chatterbox application. But this is not being done. Hence you end up with a lot of internal state which is never reclaimed. I looked at the chatterbox source code, but I have almost no idea about its internal design invariants, so it is hard to add queries to internals and make sure the invariants are always true in the system design. A good rule would be that closed streams are not in the internal list of streams anymore. Perhaps after a while when garbage collection has happened (expose an explicit GC and call it in the test case). As for the Open Sourcing of our system: We have a "notify" application sitting in another application. It is separable from the main release easily, but it requires some renaming in order to end up as open source. This application has a design which is currently quite synchronous and it can't reach really high notification speeds in its current incarnation. There is provisioning for adding more async operation however. FCM can batch operations and is thus easy to handle synchronously. You just grab 1000 notifications at a time and forget everything about round trips. But for APNs, you aren't that lucky. We do have some async operation in there to speed it up, but Apple is going to rate limit you I think. |
After recent changes in master (moving back to |
Chatterbox in version 0.5.0 has a memory leak which has been there
since at least December 2016. The problem is that finished HTTP/2
streams are not cleared out properly when you get their response. This
means the Chatterbox process will keep allocating more and more
memory as time goes on and eventually crash the Erlang node.
Use the git-version of Chatterbox for now, which has the fix in place.
This avoids memleaks of this kind.