Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ddtrace/profiling/collector/_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,17 @@ def __getattr__(self, name: str) -> Any:
return getattr(original_class, name)
raise AttributeError(f"'{type(self).__name__}' object has no attribute '{name}'")

def __mro_entries__(self, bases: Tuple[Any, ...]) -> Tuple[Type[Any], ...]:
"""Support subclassing the wrapped lock type (PEP 560).

Without this, when custom lock types inherit from a wrapped lock
(e.g. neo4j's AsyncRLock that inherits from asyncio.Lock), the program would error with:
> TypeError: _LockAllocatorWrapper.__init__() takes 2 positional arguments but 4 were given

This method returns the actual object type to be used as the base class.
"""
return (self._original_class,) # type: ignore[return-value]


class LockCollector(collector.CaptureSamplerCollector):
"""Record lock usage."""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
fixes:
- |
profiling: Fixes a bug where code that sub-classes our wrapped locks crashes with ``TypeError`` during profiling.
One example of this is neo4j's ``AsyncRLock``, which inherits from ``asyncio.Lock``:
https://github.com/neo4j/neo4j-python-driver/blob/6.x/src/neo4j/_async_compat/concurrency.py#L45
29 changes: 29 additions & 0 deletions tests/profiling/collector/test_asyncio.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,35 @@ def teardown_method(self):
except Exception as e:
print("Error while deleting file: ", e)

async def test_subclassing_wrapped_lock(self) -> None:
"""Test that subclassing of a wrapped lock type when profiling is active."""
from typing import Optional

from ddtrace.profiling.collector._lock import _LockAllocatorWrapper

with collector_asyncio.AsyncioLockCollector(capture_pct=100):
assert isinstance(asyncio.Lock, _LockAllocatorWrapper)

# This should NOT raise TypeError
class CustomLock(asyncio.Lock): # type: ignore[misc]
def __init__(self) -> None:
super().__init__()
self._owner: Optional[int] = None
self._count: int = 0

# Verify subclassing and functionality
custom_lock: CustomLock = CustomLock()
assert hasattr(custom_lock, "_owner")
assert hasattr(custom_lock, "_count")
Comment on lines +72 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to test this? Isn't it true by definition of CustomLock? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess just some sanity checks, but they're not strictly necessary. Having the CustomLock object instantiate without failing is all we really need. Do you feel strongly about removing it? I guess it would make the intention of this test clearer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I'm generally against hasattr where not strictly necessary, so in this case yeah I would advocate for removing them. Additionally, we're using them right after so if they're not there, we'll get a failure anyway.

assert custom_lock._owner is None
assert custom_lock._count == 0

# Test async acquire/release
await custom_lock.acquire()
assert custom_lock.locked()
custom_lock.release()
assert not custom_lock.locked()

async def test_asyncio_lock_events(self):
with collector_asyncio.AsyncioLockCollector(capture_pct=100):
lock = asyncio.Lock() # !CREATE! test_asyncio_lock_events
Expand Down
26 changes: 26 additions & 0 deletions tests/profiling/collector/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -1378,6 +1378,32 @@ class BaseSemaphoreTest(BaseThreadingLockCollectorTest):
particularly those related to internal lock detection and Condition-based implementation.
"""

def test_subclassing_wrapped_lock(self) -> None:
"""Test that subclassing of a wrapped lock type works when profiling is active.

This test is only valid for Semaphore-like types (pure Python classes).
threading.Lock and threading.RLock are C types that don't support subclassing
through __mro_entries__.
"""
from ddtrace.profiling.collector._lock import _LockAllocatorWrapper

with self.collector_class():
assert isinstance(self.lock_class, _LockAllocatorWrapper)

# This should NOT raise TypeError
class CustomLock(self.lock_class): # type: ignore[misc]
def __init__(self) -> None:
super().__init__()
self.custom_attr: str = "test"

# Verify subclassing and functionality
custom_lock: CustomLock = CustomLock()
assert hasattr(custom_lock, "custom_attr")
assert custom_lock.custom_attr == "test"

assert custom_lock.acquire()
custom_lock.release()

def _verify_no_double_counting(self, marker_name: str, lock_var_name: str) -> None:
"""Helper method to verify no double-counting in profile output.

Expand Down
Loading