Skip to content

fix(aioredis): missing attribute in aioredis patch #14334

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

Closed
wants to merge 1 commit into from

Conversation

valfa14
Copy link

@valfa14 valfa14 commented Aug 18, 2025

Checklist

  • PR author has checked that all the criteria below are met
  • The PR description includes an overview of the change
  • The PR description articulates the motivation for the change
  • The change includes tests OR the PR description describes a testing strategy
  • The PR description notes risks associated with the change, if any
  • Newly-added code is easy to change
  • The change follows the library release note guidelines
  • The change includes or references documentation updates if necessary
  • Backport labels are set (if applicable)

Reviewer Checklist

  • Reviewer has checked that all the criteria below are met
  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Newly-added code is easy to change
  • Release note makes sense to a user of the library
  • If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

CancelledError is not defined in aioredis
https://github.com/aio-libs-abandoned/aioredis-py/blob/master/aioredis/exceptions.py

@valfa14 valfa14 requested review from a team as code owners August 18, 2025 13:38
Copy link
Collaborator

@emmettbutler emmettbutler left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @valfa14. It would be great if you could add a regression test that illustrates the conditions under which the bug you've fixed can manifest.

@valfa14 valfa14 changed the title Fix missing attribute fix: fix missing attribute in aioredis handler Aug 19, 2025
@valfa14 valfa14 changed the title fix: fix missing attribute in aioredis handler fix: fix missing attribute in aioredis patch Aug 19, 2025
@valfa14
Copy link
Author

valfa14 commented Aug 19, 2025

@emmettbutler Hey, just quick test

import asyncio
import importlib
import sys
import unittest.mock

import pytest


def _get_traced_execute_command():
    """
    Get the traced_13_execute_command function from ddtrace aioredis patch
    to access its internal _finish_span callback.
    """
    try:
        m = importlib.import_module("ddtrace.contrib.internal.aioredis.patch")
        return getattr(m, "traced_13_execute_command", None)
    except Exception:
        return None


class _DummySpan:
    def __init__(self):
        self.finished = False
        self.exc = None

    def set_exc_info(self, *args, **kwargs):
        # ddtrace calls span.set_exc_info(*sys.exc_info())
        self.exc = args or tuple()

    def set_tag(self, *args, **kwargs):
        pass

    def finish(self):
        self.finished = True


class _MockRedisInstance:
    """Mock Redis instance to simulate the traced_execute_command call"""
    def __init__(self):
        self.address = ('localhost', 6379)
        self.db = 0
        self.connection = None  # Not a _RedisBuffer


class _MockPin:
    """Mock Pin for ddtrace"""
    def __init__(self):
        self.tracer = unittest.mock.MagicMock()
        self.tags = {}
        
    def enabled(self):
        return True


class _CancelledFuture:
    """
    Mock future that raises asyncio.CancelledError when result() is called
    """
    def result(self):
        raise asyncio.CancelledError("Task was cancelled")

    def exception(self):
        return asyncio.CancelledError("Task was cancelled")

    def cancelled(self):
        return True

    def add_done_callback(self, callback):
        # Immediately call the callback to simulate the future completing
        callback(self)


def test_aioredis_patch_references_nonexistent_cancelled_error():
    """
    Test that demonstrates the bug: ddtrace's aioredis patch tries to catch
    aioredis.CancelledError, but this attribute doesn't exist in current aioredis versions.
    """
    # First, show that aioredis.CancelledError doesn't exist
    import aioredis
    with pytest.raises(AttributeError):
        _ = aioredis.CancelledError
    
    # Now demonstrate that the ddtrace code will fail when it tries to reference it
    # We can simulate this by trying to execute the same except clause that's in the ddtrace code
    
    def simulate_finish_span_except_clause():
        """Simulates the except clause from ddtrace's _finish_span callback"""
        try:
            # Simulate an asyncio.CancelledError being raised
            raise asyncio.CancelledError("Task was cancelled")
        except (Exception, aioredis.CancelledError):  # This line will fail!
            pass
    
    # This should raise AttributeError when trying to access aioredis.CancelledError
    with pytest.raises(AttributeError, match="module 'aioredis' has no attribute 'CancelledError'"):
        simulate_finish_span_except_clause()


def test_demonstrates_missing_aioredis_cancelled_error():
    """This test demonstrates that aioredis.CancelledError doesn't exist"""
    import aioredis
    
    # This should raise AttributeError
    with pytest.raises(AttributeError, match="module 'aioredis' has no attribute 'CancelledError'"):
        _ = aioredis.CancelledError


def test_correct_fix_uses_asyncio_cancelled_error():
    """
    Test that demonstrates the correct fix: using asyncio.CancelledError instead of aioredis.CancelledError
    """
    import asyncio
    
    def corrected_finish_span_except_clause():
        """Simulates the corrected except clause that should be in ddtrace's _finish_span callback"""
        try:
            # Simulate an asyncio.CancelledError being raised
            raise asyncio.CancelledError("Task was cancelled")
        except (Exception, asyncio.CancelledError):  # This works!
            return "Caught successfully"
        except BaseException:  # Even better: catch BaseException to handle all cases
            return "Caught with BaseException"
    
    # This should work without any AttributeError
    result = corrected_finish_span_except_clause()
    assert result == "Caught successfully"
    
    # Also test that asyncio.CancelledError exists and is the right type
    assert hasattr(asyncio, 'CancelledError')
    assert issubclass(asyncio.CancelledError, BaseException)
    

@brettlangdon brettlangdon changed the title fix: fix missing attribute in aioredis patch fix(aioredis): missing attribute in aioredis patch Aug 19, 2025
@emmettbutler
Copy link
Collaborator

@valfa14 Apologies, but I noticed that this integration is deprecated. We recommend using the redis integration instead. #14356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants