Multiple fixes and enhancements for monkey pluggability #1449

arcivanov · 2019-08-15T17:11:54Z

_check_repatching now merges kwargs into state
_get_patch_all_state allows to retrieve the patch_all configuration state
3.a _patch_module now allows to specific _package_prefix parameter that defaults to 'gevent.'
3.b import_module is now used in _patch_module to allow handling of package names of any depth.
patch_item and both patch_modules now accept argument _patch_module for item's __module__ override.

1. `_check_repatching` now merges kwargs into state 2. `_get_patch_all_state` allows to retrieve the patch_all configuration state 3.a `_patch_module` now allows to specific `_package_prefix` parameter that defaults to 'gevent.' 3.b `import_module` is now used in `_patch_module` to allow handling of package names of any depth. 4. `patch_item` and both `patch_modules` now accept argument `_patch_module` for item's `__module__` override. fixes gevent#1447

jamadden · 2019-08-19T10:47:19Z

Thanks for this PR! I apologize for the delayed response, I've been fully occupied with other projects. I'll get back to this as soon as I can.

jamadden

Thanks again for this PR. I'm very interested in developing a patching API that's truly usable and useful for external plugins.

gevent master is in a good state again after the SSL updates broke things, so now on Travis we can see one test failure that seems to be related to these changes. I also asked for some clarification and discussion on some of the changes.

jamadden · 2019-09-13T12:21:28Z

src/gevent/monkey.py

    olditem = getattr(module, attr, _NONE)
    if olditem is not _NONE:
        saved.setdefault(module.__name__, {}).setdefault(attr, olditem)
    setattr(module, attr, newitem)
+    if _patch_module:


I'm not sure this is a good idea in general. One example that springs to mind is pickling; another is that it may tend to produce very confusing repr results. Why would we want to do this?

If this is something that's for very specialized use-cases, perhaps that would be better served in the hook functions for the module? (See below for some general comments about underscore-prefixed things.)

Reviewing #1447 I see that pickling was specifically mentioned as the motivation for this. I'm still not convinced, I'd need to see a specific example to understand what problem this solves that can't be solved by the object itself.

jamadden · 2019-09-13T12:39:11Z

src/gevent/monkey.py

@@ -119,6 +119,7 @@ def patch_psycopg(event):
 from __future__ import absolute_import
 from __future__ import print_function
 import sys
+from importlib import import_module


I was concerned about this extra import either leading to more imports that might cause issue with monkey-patching, or with it taking time for cases that we might not need it.

But on Python 3, it's imported by default anyway during startup, so those are completely moot.

On Python 2, it's a trivial file with no imports besides sys.

jamadden · 2019-09-13T12:43:39Z

src/gevent/monkey.py

-                  _call_hooks=True):
+                  _call_hooks=True,
+                  _patch_module=False,
+                  _package_prefix='gevent.'):


In the tradition of 'explicit is better than implicit', it would probably be better to make this a required argument instead of defaulting.

Instead of an extra argument, what if name was required to be the fully-qualified name instead of the simple module name? That would allow patches that consist only of a module, not a package. (With a small amount of work, that could generalize to allowing arbitrary objects to be the patch source.)

I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).

jamadden · 2019-09-13T12:43:51Z

src/gevent/monkey.py


-    gevent_module = getattr(__import__('gevent.' + name), name)
+    gevent_module = import_module(_package_prefix + name)


Probably better named source_module at this point.

I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).

jamadden · 2019-09-13T12:52:10Z

src/gevent/monkey.py

@@ -951,9 +964,15 @@ def patch_signal():
    _patch_module("signal")


+def _get_patch_all_state():


This function doesn't seem to be used here.

Here's my general philosophy about the underscore prefixed functions and arguments in this module: They're an implementation detail, subject to change. (That's obvious.) But we also know that, as external plugins get going, until we feel ready to publish some sort of official monkey-patching API, they'll be used outside the gevent package. If functionality that gevent itself doesn't use is only used by those external plugins, its much more likely that that functionality is going to break in a way that is not caught by the gevent tests, or be refactored away without planning too. So it's best to keep the implementation details to a minimum and be sure that gevent has a use-case for them all.

In this case, it looks like it would be easy to make _check_repatching use this function.

This function is supposed to be used externally by plugins. I was hesitant to make it public, but then again maybe I should've.

I have no problem with any aesthetic API changes (argument requirement, order, naming, refactoring).

jamadden · 2019-09-13T12:55:35Z

src/gevent/monkey.py

 def _check_repatching(**module_settings):
    _warnings = []
    key = '_gevent_saved_patch_all'
+    module_settings.update(module_settings['kwargs'])


It's not clear to me what difference this makes. Could you clarify please? It may be missing a test case, or perhaps that's the reason for the broken test.

Oh. Silly me. I just reviewed #1447 and see where the problem was mentioned. So this either is the reason for the test failure, or it definitely needs a specific test.

arcivanov force-pushed the issue_1447 branch from 3620603 to b49b7c5 Compare August 15, 2019 17:12

jamadden reviewed Sep 13, 2019

View reviewed changes

jamadden force-pushed the master branch 2 times, most recently from 56b1951 to 352434b Compare March 23, 2020 16:51

jamadden force-pushed the master branch 3 times, most recently from bae7acd to d29d50a Compare April 16, 2020 23:29

jamadden force-pushed the master branch from 4b8eabb to bf4e5d8 Compare June 10, 2020 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple fixes and enhancements for monkey pluggability #1449

Multiple fixes and enhancements for monkey pluggability #1449

arcivanov commented Aug 15, 2019

jamadden commented Aug 19, 2019

jamadden left a comment

jamadden Sep 13, 2019

jamadden Sep 13, 2019

jamadden Sep 13, 2019

jamadden Sep 13, 2019

arcivanov Sep 21, 2019

jamadden Sep 13, 2019

arcivanov Sep 21, 2019

jamadden Sep 13, 2019

arcivanov Sep 21, 2019

jamadden Sep 13, 2019

jamadden Sep 13, 2019


		gevent_module = getattr(__import__('gevent.' + name), name)
		gevent_module = import_module(_package_prefix + name)

		@@ -951,9 +964,15 @@ def patch_signal():
		_patch_module("signal")


		def _get_patch_all_state():

Multiple fixes and enhancements for monkey pluggability #1449

Are you sure you want to change the base?

Multiple fixes and enhancements for monkey pluggability #1449

Conversation

arcivanov commented Aug 15, 2019

jamadden commented Aug 19, 2019

jamadden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment