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

Fix for select read+write and spurious wakeup #552

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

erickj00001
Copy link

Addresses #551

The green select function relies on removing the listeners in the "finally" block to ensure that no other callbacks will be issued. But in some hubs (poll and epolls, but not selects), callbacks are queued up in a local variable (called "callbacks"), so will not be prevented even if we remove the corresponding listeners. So in the case where both read and write are applicable, one of those callbacks gets issued after the select call returns, causing a spurious wakeup.

This addresses the issue by scheduling another callback to call current.switch, so that Hub.wait has a chance to finish issuing all callbacks.

This also has the benefit that select will now return both the read and write flags if they both apply, which is more consistent with the behavior of the original select function.

@codecov-io
Copy link

Codecov Report

Merging #552 into master will increase coverage by <1%.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #552    +/-   ##
======================================
+ Coverage      46%    46%   +<1%     
======================================
  Files          81     81            
  Lines        7978   7987     +9     
  Branches     1364   1365     +1     
======================================
+ Hits         3722   3735    +13     
+ Misses       3999   3996     -3     
+ Partials      257    256     -1
Flag Coverage Δ
#ipv6 15% <0%> (-1%) ⬇️
#py27epolls 50% <100%> (ø) ⬆️
#py27poll 50% <100%> (ø) ⬆️
#py27selects 49% <100%> (ø) ⬆️
#py34epolls 42% <100%> (ø) ⬆️
#py34poll 42% <100%> (ø) ⬆️
#py34selects 42% <100%> (ø) ⬆️
#py35epolls 42% <100%> (ø) ⬆️
#py35poll 42% <100%> (ø) ⬆️
#py35selects 42% <100%> (ø) ⬆️
#py36epolls 42% <100%> (ø) ⬆️
#py36poll 42% <100%> (ø) ⬆️
#py36selects 42% <100%> (ø) ⬆️
#py37epolls 42% <100%> (ø) ⬆️
#py37poll 42% <100%> (ø) ⬆️
#py37selects 42% <100%> (ø) ⬆️
Impacted Files Coverage Δ
eventlet/green/select.py 97% <100%> (+5%) ⬆️
eventlet/hubs/poll.py 82% <0%> (-2%) ⬇️
eventlet/hubs/hub.py 91% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a915bb6...f0b5e1b. Read the comment docs.

@thedrow
Copy link
Contributor

thedrow commented Mar 18, 2019

Can we please merge and release this as soon as possible?
#551 is hurting Celery users.

@dralley
Copy link

dralley commented Apr 16, 2019

RQ users also, via redis-py

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

Successfully merging this pull request may close these issues.

None yet

5 participants