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

Fixes #7052 gevent/eventlet 1 seconds latency #7089

Merged
merged 2 commits into from Nov 15, 2021

Conversation

mrmaxi
Copy link
Contributor

@mrmaxi mrmaxi commented Nov 14, 2021

Issue #7052
When there are used asynchronous backend as Redis with pubsub, result of short task (0.1s length, for example) available immediate.

When execute AsyncResult.get() under gevent/eventlet environment, result of the same short (0.1s) tasks became available after 1s, it is too slowely.

Current behavior is result of GH-5974 that prevent event loop blocking by very offen checking task results and very short wait_for length.
Current implementation of greenletDrainer cause wait_for to only return every "timeout" # of seconds, rather than returning immediately.

Solutuion:
I add new internal event _drain_complete_event that rising every time after socket operation result_consumer.drain_events (for example _pubsub.get_message for Redis backend) occurs.
Every wait_for are waiting for this particular event, so it won't cause overhead, because socket operation still not block event loop and let other greenlets executing normaly.

mrmaxi and others added 2 commits November 14, 2021 15:46
Wakeup waiters in `wait_for` after every `drain_events` occurs instead of only after 1 seconds timeout.

Does not block event loop, because `drain_events` of asynchronous backends with pubsub commonly sleeping for some nonzero time while waiting events.
@codecov
Copy link

codecov bot commented Nov 14, 2021

Codecov Report

Merging #7089 (366258e) into master (fe37cd8) will increase coverage by 0.01%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7089      +/-   ##
==========================================
+ Coverage   89.35%   89.37%   +0.01%     
==========================================
  Files         138      138              
  Lines       16744    16759      +15     
  Branches     2212     2211       -1     
==========================================
+ Hits        14962    14978      +16     
- Misses       1551     1552       +1     
+ Partials      231      229       -2     
Flag Coverage Δ
unittests 89.36% <87.50%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
celery/backends/asynchronous.py 69.26% <87.50%> (+2.59%) ⬆️

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 fe37cd8...366258e. Read the comment docs.

@lgtm-com
Copy link

lgtm-com bot commented Nov 14, 2021

This pull request introduces 1 alert and fixes 2 when merging 366258e into fe37cd8 - view on LGTM.com

new alerts:

  • 1 for `__init__` method calls overridden method

fixed alerts:

  • 2 for Wrong name for an argument in a call

@auvipy auvipy added this to the 5.2.x milestone Nov 14, 2021
@auvipy
Copy link
Member

auvipy commented Nov 14, 2021

thank you, will look into this tomorrow

@auvipy auvipy self-requested a review November 14, 2021 14:08
@auvipy auvipy merged commit 59f2271 into celery:master Nov 15, 2021
@auvipy auvipy linked an issue Nov 15, 2021 that may be closed by this pull request
4 tasks
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.

AsyncResult.get() has at least 1 second latency while executing under gevent
2 participants