Skip to content
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

Only set disconnected time when it is not already set. #237

Merged
merged 5 commits into from
Nov 27, 2018

Conversation

agronick
Copy link
Contributor

This is fix 1 of 2 for django/channels#1181.

@andrewgodwin
Copy link
Member

So looking at this change in isolation, I'm curious what effect it would have - did you write it anticipating solving a race condition, or something else? I'd like to see a comment with it because otherwise it looks like you're avoiding replacing a number with another similar-size number which would not really affect memory.

Nov 27 13:20:47 python3[28665]: File "/var/lib/wwwrun/.virtualenvs//lib/python3.6/site-packages/channels/consumer.py", line 81, in send
Nov 27 13:20:47 python3[28665]: await self.base_send(message)
Nov 27 13:20:47 python3[28665]: File "/var/lib/wwwrun/.virtualenvs//lib/python3.6/site-packages/channels/sessions.py", line 232, in send
Nov 27 13:20:47 python3[28665]: return await self.real_send(message)
Nov 27 13:20:47 python3[28665]: File "/var/lib/wwwrun/.virtualenvs//lib/python3.6/site-packages/daphne/server.py", line 213, in handle_reply
Nov 27 13:20:47 python3[28665]: if self.connections[protocol].get("disconnected", None):
Nov 27 13:20:47 python3[28665]: KeyError: <WebSocketProtocol client=None path=b'/vnc/devices/'>
This reverts commit f0c9878.
@agronick
Copy link
Contributor Author

agronick commented Nov 27, 2018

I wrote the details in the bug thread. Avoiding writing a bigger number is what we are doing because if it does the time difference will never be large enough to be cleaned up.

  • check_timeouts() runs every two seconds
  • It calls finish() which calls self.server.protocol_disconnected(self)
  • t keeps overwriting the time of the disconnected key
  • The disconnected time difference is never high enough to get cleaned up:
            if (
                disconnected
                and time.time() - disconnected > self.application_close_timeout
            ):
  • check_timeouts() continues to be called every two seconds.
  • It keeps calling send_disconnect() which causes the memory leak with this code:
            self.application_queue.put_nowait({"type": "http.disconnect"})
  • This makes a giant queue full of useless data.

To experience this just make a view that has time.sleep(999999) with a low http_timeout.

@agronick
Copy link
Contributor Author

agronick commented Nov 27, 2018

I added another fix that handles an exception where send is called on an application instance that has been cleaned up. I see that exception a few times a day. I added comments for both changes.

@andrewgodwin andrewgodwin merged commit c4125c6 into django:master Nov 27, 2018
@andrewgodwin
Copy link
Member

Great, thanks for adding the comment!

@agronick
Copy link
Contributor Author

Any idea when this is going out? I had this bug hit last night and this is what it did to my memory usage:

image

@andrewgodwin
Copy link
Member

I'll try to get a release out this week - I forgot to roll one along with Channels last week.

@andrewgodwin
Copy link
Member

It's now out in 2.2.4.

@agronick
Copy link
Contributor Author

agronick commented Dec 16, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants