calling eventlib/gevent patching by avikam · Pull Request #6322 · celery/celery

avikam · 2020-08-30T20:41:43Z

Description

eventlet gPidbox is not running in 5.0.0rc1, as described here: #6225

Following the move to Click (https://github.com/celery/celery/pull/5718), I noticed that the patching code was commented out.
I'm not sure if the proposed way is a valid way to port the import patching, or the maintainers had other design in mind, but this fix is working for me.

lgtm-com · 2020-08-30T20:58:36Z

This pull request introduces 1 alert when merging b4c82a7 into 5ab26db - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-08-30T21:32:06Z

This pull request introduces 1 alert when merging de1bf7d into 5ab26db - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause

lgtm-com · 2020-08-31T01:17:21Z

This pull request introduces 1 alert and fixes 1 when merging 32ee9f3 into 5ab26db - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause

fixed alerts:

1 for Non-exception in 'except' clause

lgtm-com · 2020-08-31T01:58:00Z

This pull request fixes 1 alert when merging dc8d20b into 5ab26db - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause

avikam · 2020-08-31T13:46:45Z

looks like it's too late, i got a max-recursion error trying to open an ssl connection.
drafting for now

lgtm-com · 2020-08-31T15:16:18Z

This pull request introduces 1 alert and fixes 1 when merging af5d81c into 5fb1393 - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause

fixed alerts:

1 for Non-exception in 'except' clause

auvipy · 2020-08-31T15:36:06Z

seems lazy app passing

avikam · 2020-08-31T16:06:40Z

seems lazy app passing

yes, now i'm thinking how can I port the user_options feature, since it requires instantiating the app on celery command which might be too early. (https://github.com/celery/celery/blob/master/celery/bin/celery.py#L110)
any idea how can I read this configuration without importing the entire app?

lgtm-com · 2020-08-31T16:12:18Z

This pull request introduces 2 alerts and fixes 1 when merging 09403a7 into 5806976 - view on LGTM.com

new alerts:

1 for Non-exception in 'except' clause
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Non-exception in 'except' clause

lgtm-com · 2020-08-31T19:30:43Z

This pull request fixes 1 alert when merging cd48206 into 5806976 - view on LGTM.com

fixed alerts:

1 for Non-exception in 'except' clause

avikam · 2020-09-01T04:16:56Z

This is still a draft, @auvipy, @thedrow - i'm happy to hear your feedback.

The idea of this change is to instantiate the app lazily so that eventlet/gevent will patch the standard library before the app is imported. To achieve that I used a cached property on the click-context.
However inside the main celery command (before bin.worker is evaluated), there is a reference to the app, which adds additional user options to worker+beat+event commands (side note - the documentation mentions argparse which I assume should be changed to Click?)
So to circumvent this reference, I manually parse the residual args that complies with user options on the worker command.
There is more work left to do, for example to project these user options on the help page, but I think it's achievable.
I was able to run a celery worker with this app:

from celery import Celery
from celery.bin.base import CeleryOption

app = Celery('tasks', backend='redis://localhost', broker='redis://localhost')

app.user_options['worker'].add(CeleryOption(('-z', '--hello'), help_group="User Options"))
app.user_options['worker'].add(CeleryOption(('--flag', ), is_flag=True, help_group="User Options"))

and with this commandline:
celery -A myapp worker -P eventlet -c 10 broker_url=redis:// --hello world --flag

thedrow · 2020-09-01T10:20:27Z

Oh I completely forgot about gevent/eventlet patching.
It's good that you noticed.

The documentation wasn't updated yet. I'll read the entire issue first before I respond more thoroughly.

thedrow

Some issues here are minor, some are major.

This is a release blocker for 5.0.0 GA so we need to urgently resolve this.

thedrow · 2020-09-01T11:08:57Z

+    @property
+    @functools.lru_cache(None)
+    def app(self):
+        return self._app() or get_current_app()


Please provide a docstring which explains what happens here.

thedrow · 2020-09-01T11:09:31Z

-    worker.params.extend(ctx.obj.app.user_options.get('worker', []))
-    beat.params.extend(ctx.obj.app.user_options.get('beat', []))
-    events.params.extend(ctx.obj.app.user_options.get('events', []))
+    # worker.params.extend(ctx.obj.app.user_options.get('worker', []))


We need this to support preload options.

a compensation for that is done here:
https://github.com/celery/celery/pull/6322/files#diff-a6e64fe5a7c24a0e587e8c92e83b87b5R321

(of course, it is only a draft as it needs to be applied on beat and events as well. i wanted to get an approval for this idea before pursuing it)

there is a downside to this approach, but i'm not sure how else to get the user options only after the patching

thedrow · 2020-09-01T11:12:21Z

    def convert(self, value, param, ctx):
        # Pools like eventlet/gevent needs to patch libs as early
        # as possible.
+        maybe_patch_concurrency(value)


I'm pretty sure that's the wrong hook for this.

I'm open for suggestions.
My reasoning was to make it as close as possible to -P evaluation which is the only indication that patching is needed.

A possibility is to move this call to the command function itself, but then we can't reference the app in any of the click.options (bc it will cause the app to be imported and the patching to be too late)

thedrow · 2020-09-01T11:13:01Z

              '--statedb',
              cls=CeleryOption,
              type=click.Path(),
-              callback=lambda ctx, _, value: value or ctx.obj.app.conf.worker_state_db,


This cannot be removed. The default value must be derived from the app if it is not supplied.

It's not removed, only moved below, so the app will not be loaded too soon. See the call to worker.start in this function

Let's address the other issues first and see if we can find the way to write this code as Click intended it to be written.

lgtm-com · 2020-09-01T12:23:34Z

This pull request introduces 1 alert and fixes 1 when merging d2def45 into 6248d87 - view on LGTM.com

new alerts:

1 for Unused import

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-09-01T13:03:13Z

This pull request fixes 1 alert when merging 7f650c3 into 6248d87 - view on LGTM.com

fixed alerts:

1 for Module is imported with 'import' and 'import from'

thedrow · 2020-09-01T13:54:13Z

diff --git a/celery/__init__.py b/celery/__init__.py
index 03a69a6b8..f2e406a9a 100644
--- a/celery/__init__.py
+++ b/celery/__init__.py
@@ -112,12 +112,6 @@ def _patch_gevent():
     import gevent.signal
 
     gevent.monkey.patch_all()
-    if gevent.version_info[0] == 0:  # pragma: no cover
-        # Signals aren't working in gevent versions <1.0,
-        # and aren't monkey patched by patch_all()
-        import signal
-
-        signal.signal = gevent.signal
 
 
 def maybe_patch_concurrency(argv=None, short_opts=None,
diff --git a/celery/__main__.py b/celery/__main__.py
index b0557b185..e865ea4bd 100644
--- a/celery/__main__.py
+++ b/celery/__main__.py
@@ -2,15 +2,15 @@
 
 import sys
 
-# from . import maybe_patch_concurrency
+from . import maybe_patch_concurrency
 
 __all__ = ('main',)
 
 
 def main():
     """Entrypoint to the ``celery`` umbrella command."""
-    # if 'multi' not in sys.argv:
-    #     maybe_patch_concurrency()
+    if 'multi' not in sys.argv:
+        maybe_patch_concurrency()
     from celery.bin.celery import main as _main
     sys.exit(_main())

Does this patch also work? Can you try to git apply this on master?
It is much simpler and reuses the old code.

avikam · 2020-09-01T14:54:33Z

yep, i thought the idea was to leave all argv responsibility to click, but this code works.
closing.

thedrow · 2020-09-01T15:14:06Z

yep, i thought the idea was to leave all argv responsibility to click, but this code works.
closing.

Thank you for bringing this to our attention. 😺
Support for gevent and eventlet is going away in 6.0 anyway as we migrate to an async event loop which does not support them.
I'd rather not touch whatever works.

I'm going to release a new RC in a few moments.
Please try it out :)

moving eventlib patching to Click

b4c82a7

avikam force-pushed the master branch from 943ee41 to de1bf7d Compare August 30, 2020 21:11

avikam force-pushed the master branch from de1bf7d to 32ee9f3 Compare August 31, 2020 01:06

fixed tests

dc8d20b

avikam force-pushed the master branch from 32ee9f3 to dc8d20b Compare August 31, 2020 01:43

avikam marked this pull request as ready for review August 31, 2020 01:45

auvipy requested a review from thedrow August 31, 2020 07:00

auvipy added this to the 5.0.0 milestone Aug 31, 2020

avikam marked this pull request as draft August 31, 2020 13:37

lazy app - will it fix patching?

af5d81c

remove callbacks because we need to guarentee the app is imported last

09403a7

comma

cd48206

auvipy self-requested a review September 1, 2020 01:50

parsing user options separately

c488cc4

thedrow mentioned this pull request Sep 1, 2020

Release 5.0 #6266

Closed

23 tasks

thedrow suggested changes Sep 1, 2020

View reviewed changes

thedrow added Component: CLI PR Type: Bugfix labels Sep 1, 2020

thedrow added Priority: Blocker Severity: Blocker Version: 5.0 labels Sep 1, 2020

thedrow and others added 2 commits September 1, 2020 14:52

Remove compatibility code for gevent<1.0.

54356fb

eager should enforce the pool option to be evaluated first

d2def45

avikam added 3 commits September 1, 2020 08:29

added docstring

6dbcc13

minor

bd95609

minor

7f650c3

avikam closed this Sep 1, 2020

anilaratna2 mentioned this pull request Jun 12, 2025

maybe_patch_concurrency not called for -P gevent and hence monkey_patching.all() did not happen for gevent #9756

Closed

19 tasks

Uh oh!

Uh oh!

Conversation

avikam commented Aug 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

lgtm-com Bot commented Aug 30, 2020

Uh oh!

lgtm-com Bot commented Aug 30, 2020

Uh oh!

lgtm-com Bot commented Aug 31, 2020

Uh oh!

lgtm-com Bot commented Aug 31, 2020

Uh oh!

avikam commented Aug 31, 2020

Uh oh!

lgtm-com Bot commented Aug 31, 2020

Uh oh!

auvipy commented Aug 31, 2020

Uh oh!

avikam commented Aug 31, 2020

Uh oh!

lgtm-com Bot commented Aug 31, 2020

Uh oh!

lgtm-com Bot commented Aug 31, 2020

Uh oh!

avikam commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thedrow commented Sep 1, 2020

Uh oh!

thedrow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thedrow Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

thedrow Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

avikam Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thedrow Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

avikam Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

thedrow Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

avikam Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

thedrow Sep 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lgtm-com Bot commented Sep 1, 2020

Uh oh!

lgtm-com Bot commented Sep 1, 2020

Uh oh!

thedrow commented Sep 1, 2020

Uh oh!

avikam commented Sep 1, 2020

Uh oh!

thedrow commented Sep 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

avikam commented Aug 30, 2020 •

edited

Loading

avikam commented Sep 1, 2020 •

edited

Loading

avikam Sep 1, 2020 •

edited

Loading