-
Notifications
You must be signed in to change notification settings - Fork 197
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
respect channel layer capacity in RedisChannelLayer.receive_buffer #219
respect channel layer capacity in RedisChannelLayer.receive_buffer #219
Conversation
55e203f
to
a623e78
Compare
Hey @ryanpetrello — super stuff. I'm on 🏝 — give me a week or so to respond properly. Thanks! |
Hey @carltongibson, I'm jealous 😄 enjoy! |
Hey @carltongibson, Any chance you've had some time to look over this PR? I have a number of Ansible AWX users that are using a version of this patch, and it looks promising (none have yet reported back the memory leak after applying this patch; I've got at least one user who was seeing memory spikes every 48 hours and was restarting Daphne, and has now not reported any issues for nearly 3 weeks). |
Hi @ryanpetrello. I'm just back from holiday today (and had three versions of Django out this morning). I'll try and give it a look this week. Thanks for the hustle! 😀 |
Awesome - thanks! |
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.
Hi @ryanpetrello — I know this is a slow leak but do you think you can add some basic regression tests of the the new queue?
Hey @carltongibson - what sort tests are you looking for? Something that verifies that the queue won't grow beyond the configured capacity? |
Yeah, that sounds fine. "Did the basic behaviour break?" is enough. |
a623e78
to
d3c663b
Compare
@carltongibson test added - let me know if this is what you're looking for. |
0037e2a
to
b6afb2a
Compare
@ryanpetrello Quick glance, looks about right. Thank you. I'll have one more look over the issues and then we'll bundle this up for a release. 👍 Thanks for your input here! 🥇 |
b6afb2a
to
6361946
Compare
@@ -544,7 +560,11 @@ async def new_channel(self, prefix="specific"): | |||
Returns a new channel name that can be used by something in our | |||
process as a specific channel. | |||
""" | |||
return "%s.%s!%s" % (prefix, self.client_prefix, uuid.uuid4().hex,) | |||
return "%s.%s!%s" % ( |
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.
@carltongibson I changed this because black
was complaining about it style-wise
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.
No problem. Whatever black says. 😀
6361946
to
e50bb5a
Compare
respect the capacity setting so that the receive_buffer does not grow without bounds see: django#212
e50bb5a
to
64b258f
Compare
cc @carltongibson
respect the capacity setting so that the receive_buffer does not grow
without bounds (which can lead to a memory leak)
see: #212
based on the idea outlined here:
#384