-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix 100% CPU usage #875
Fix 100% CPU usage #875
Conversation
@jaredsuttles Thanks for this patch! It looks like the CI failure is unrelated (#873). There's another poller bug reported in #858. Does this patch happen to fix that as well, and if not, could you possibly look at it as well? |
CI failure is unrelated and should be fixed since fa7e3f5. |
@jaredsuttles Thanks much for the #885. I will test and intend to apply it if there aren't issues. Can you please check out #903? It is another patch that aims to fix the same issue as this one, but it's completely different. I could use help figuring out the best thing to do here. |
Reading @jaredsuttles code, I think he has a better fix than mine. So I think this one should go in. |
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.
Looks great! Can we get this in? The bug is really irritating...
Could you please give me some more specific details on how they are broken so that we can create tickets for them? Do you think it would be better to just revert the poller patch entirely? It was added only recently and prior to it, |
@mnaberez Sorry, I simply meant the other pollers have the same 100% CPU usage issue so they would also need to be dealt with were you to use #903. You can test this by just setting for instance I haven't really looked at the code prior to that merge so I can't really say one way or the other. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@jaredsuttles Thanks for the clarification above on #903. I hope to keep the kqueue poller, otherwise we have to reopen the other issues it was implemented to address. I will test this patch and #885 locally soon and if there are no issues, I'll try to make a bug fix release with them. |
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.
I'm unclear on the changes to PollPoller
in unregister_readable()
and unregister_writable()
.
self._poller.unregister(fd) | ||
if fd in self.writables: | ||
self._poller.register(fd, self.WRITE) |
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.
If I am reading this correctly, it seems like unregistering a readable causes all writables to be (re)registered(?). Could you please explain this? Same thing in unregister_writable
below.
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.
In kqueue you can unregister a file descriptor for specifically read or write but, as far as I can tell, you cannot do that with poll. The workaround is to just reregister for write if we unregistered read and vice versa.
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.
Got it, thanks for clarifying.
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.
Looks good. I'll test this locally and will merge it if I don't find any issues.
@FAKERINHEART I'm not able to reproduce. This doesn't happen on master? |
@FAKERINHEART I'm testing this change locally and I'm also not able to see the error you posted. Can you please help us reproduce it or let us know if you're no longer seeing it? |
It was my own mistake for the 'BadStatusLine' error. I am sorry to forget to reply this in time. |
No problem, thanks for letting us know! |
When can we get this merged @mnaberez ? |
I tested this patch locally and it worked for me. I plan to merge and release it next week. |
@plockaby tried this patch and said in #807 (comment):
|
I feel it may be prudent to add to this thread that this 100% cpu thing only started happening when I added |
I agree that it is separate. Don't let my problem hold up merging this pull request. |
This is what I tried to reproduce the high CPU usage: Minimal
Start
Open a web browser to I was able to see this on macOS Sierra (10.12.4) with Python 2.7.13 using the current master (3d128e4) and applying this patch fixed it. I also tried this on Ubuntu Desktop 16.04.1 LTS with Python 2.7.12. On that system, I wasn't able to see the problem before or after the patch. |
@plockaby I'm also seeing poll craziness (very similar looking strace) and 100% CPU usage, on a linux rackspace box. did you open a separate issue for the issue you found? I'm running 3.3.3 fwiw, python 2.7.6 |
I ran into this bug using the web UI on macOS. It seems that #589 was on the right track but has unnecessarily strict rules to unregister a file descriptor. Only need to check if the writers are not writeable anymore - readers not readable.