Skip to content

Commit

Permalink
chore(internal): better support for Python legacy versions in ModuleW…
Browse files Browse the repository at this point in the history
…atchdog (#4083)

## Description

This change improves support for Python legacy versions, like 2.7 and 3.5 by using a common loader wrapper that makes more attributes available upon request, e.g. is_package. Furthermore, this change also provides support for dict copy via the dict constructor, which is required by some pytest fixtures, like testdir.

### More technical details

In Python<=3.5, calling the `dict` constructor on a dictionary makes a copy of it by first checking if it has a `keys` attribute, and then performing a `PyDict_Check`. If these checks succeed, the dictionary is copied using the C API. Since `ModuleWatchdog` is a subclass of `dict`, this means that the wrapping logic is bypassed, and `dict` ends up copying the dictionary backing `ModuleWatchdog` rather than the wrapped `sys.modules`. We exploit the `keys` attribute access to then copy the state of `sys.modules` over to the backing `dict`, so that calling `dict` on a `ModuleWatchdog` instance actually creates a copy of the wrapped dictionary instead of the backing one.

### Testing

This change is a pre-requisite for #4070 to make the test suite pass.

## Checklist
- [x] Title must conform to [conventional commit](https://github.com/conventional-changelog/commitlint/tree/master/%40commitlint/config-conventional).
- [x] Add additional sections for `feat` and `fix` pull requests.
- [x] Ensure tests are passing for affected code.
- [x] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) and/or [Datadog's documentation site](https://github.com/DataDog/documentation/) is updated. Link to doc PR in description.


## Reviewer Checklist
- [ ] Title is accurate.
- [ ] Description motivates each change.
- [ ] No unnecessary changes were introduced in this PR.
- [ ] PR cannot be broken up into smaller PRs.
- [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary.
- [ ] Tests provided or description of manual testing performed is included in the code or PR.
- [x] Release note has been added for fixes and features, or else `changelog/no-changelog` label added.
- [ ] All relevant GitHub issues are correctly linked.
- [ ] Backports are identified and tagged with Mergifyio.
- [ ] Add to milestone.
  • Loading branch information
P403n1x87 authored Aug 16, 2022
1 parent 82b7c7e commit 2f3d954
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 17 deletions.
48 changes: 31 additions & 17 deletions ddtrace/internal/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,26 +122,24 @@ def _resolve(path):
# https://github.com/GrahamDumpleton/wrapt/blob/df0e62c2740143cceb6cafea4c306dae1c559ef8/src/wrapt/importer.py

if PY2:
import pkgutil

find_spec = ModuleSpec = None
Loader = object

find_loader = pkgutil.find_loader

else:
from importlib.abc import Loader
from importlib.machinery import ModuleSpec
from importlib.util import find_spec

def find_loader(fullname):
# type: (str) -> Optional[Loader]
return getattr(find_spec(fullname), "loader", None)

# DEV: This is used by Python 2 only
class _ImportHookLoader(object):
def __init__(self, callback):
# type: (Callable[[ModuleType], None]) -> None
self.callback = callback

def load_module(self, fullname):
# type: (str) -> ModuleType
module = sys.modules[fullname]
self.callback(module)

return module
LEGACY_DICT_COPY = sys.version_info < (3, 6)


class _ImportHookChainedLoader(Loader):
Expand Down Expand Up @@ -283,6 +281,18 @@ def __delitem__(self, name):

def __getattribute__(self, name):
# type: (str) -> Any
if LEGACY_DICT_COPY and name == "keys":
# This is a potential attempt to make a copy of sys.modules using
# dict(sys.modules) on a Python version that uses the C API to
# perform the operation. Since we are an instance of a dict, this
# means that we will end up looking like the empty dict, so we take
# this chance to actually look like sys.modules.
# NOTE: This is a potential source of memory leaks. However, we
# expect this to occur only on defunct Python versions, and only
# during special code executions, like test runs.
super(ModuleWatchdog, self).clear()
super(ModuleWatchdog, self).update(self._modules)

try:
return super(ModuleWatchdog, self).__getattribute__("_modules").__getattribute__(name)
except AttributeError:
Expand All @@ -308,16 +318,20 @@ def find_module(self, fullname, path=None):
self._finding.add(fullname)

try:
if PY2:
__import__(fullname)
return _ImportHookLoader(self.after_import)

loader = getattr(find_spec(fullname), "loader", None)
loader = find_loader(fullname)
if loader is not None:
if not isinstance(loader, _ImportHookChainedLoader):
loader = _ImportHookChainedLoader(loader)

loader.add_callback(type(self), self.after_import)
if PY2:
# With Python 2 we don't get all the finders invoked, so we
# make sure we register all the callbacks at the earliest
# opportunity.
for finder in sys.meta_path:
if isinstance(finder, ModuleWatchdog):
loader.add_callback(type(finder), finder.after_import)
else:
loader.add_callback(type(self), self.after_import)

return loader

Expand Down
17 changes: 17 additions & 0 deletions tests/internal/test_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,3 +310,20 @@ class Bob(BaseCollector):

Bob.uninstall()
Alice.uninstall()


def test_module_watchdog_dict_shallow_copy():
# Save original reference to sys.modules
original_sys_modules = sys.modules

ModuleWatchdog.install()

# Ensure that we have replaced sys.modules
assert original_sys_modules is not sys.modules

# Make a shallow copy of both using the dict constructor
original_modules = set(dict(original_sys_modules).keys())
new_modules = set(dict(sys.modules).keys())

# Ensure that they match
assert original_modules == new_modules

0 comments on commit 2f3d954

Please sign in to comment.