Skip to content

Conversation

@DominicOram
Copy link
Contributor

Fixes #1122

To test:

  • Confirm new test fails on main but passes here

vredchenko pushed a commit to vredchenko/ophyd-async that referenced this pull request Nov 12, 2025
Review covers the fix for set_and_wait_for_other_value TimeoutError handling
when matchers lack __name__ attribute. Identifies critical issue with object
mutation and recommends using getattr() instead.
@vredchenko
Copy link

This was auto-generated by Claude Code:


PR Review

Thank you for fixing this issue! The PR successfully addresses the bug where matchers without __name__ attributes would cause an AttributeError instead of a proper TimeoutError.

Issues Found

🔴 Critical: Mutating the matcher object

The current approach mutates the matcher object by setting matcher.__name__:

except TimeoutError as exc:
    if not hasattr(matcher, "__name__"):
        matcher.__name__ = "supplied_function"  # ⚠️ Mutates the object
    raise TimeoutError(...)

Problems:

  • Creates unexpected side effects on the matcher object
  • Could affect code that reuses the same matcher instance
  • Potential race conditions with concurrent calls using the same matcher

Recommended fix: Use getattr() to avoid mutation:

except TimeoutError as exc:
    matcher_name = getattr(matcher, "__name__", "<callable>")
    raise TimeoutError(
        f"{match_signal.name} value didn't match value from"
        f" {matcher_name}() in {timeout}s"
    ) from exc

This is:

  • ✅ Safer (no side effects)
  • ✅ Cleaner (more Pythonic)
  • ✅ Simpler (fewer lines)
  • ✅ Functionally equivalent

🟡 Medium: Consider using type information

The fallback name "supplied_function" is generic. Consider using type(matcher).__name__ for more context:

matcher_name = getattr(matcher, "__name__", f"<{type(matcher).__name__}>")

This would produce error messages like:

  • "match_signal value didn't match value from <NoNameCallable>() in 0.01s"
  • "match_signal value didn't match value from <partial>() in 0.01s"

Which provides more debugging information.

Test Suggestions

Add test for functools.partial

Since issue #1122 specifically mentioned functools.partial as the trigger, consider adding an explicit test:

from functools import partial
 
async def test_partial_matcher_timeout_error():
    """Test that functools.partial matcher properly raises TimeoutError"""
    set_signal = epics_signal_rw(int, "pva://signal")
    match_signal = epics_signal_rw(int, "pva://match_signal")
 
    await set_signal.connect(mock=True)
    await match_signal.connect(mock=True)
 
    with pytest.raises(asyncio.TimeoutError):
        await set_and_wait_for_other_value(
            set_signal, 20, match_signal, partial(lambda x, y: x == y, 20), timeout=0.01
        )

Optional: Assert error message format

with pytest.raises(asyncio.TimeoutError, match=r"match_signal value didn't match"):
    await set_and_wait_for_other_value(...)

Positive Aspects

✅ Correctly identifies and fixes the core issue
✅ Good test coverage with realistic scenario
✅ Minimal, focused change
✅ Preserves exception chain with from exc

Recommendation

Approve with changes - The fix works, but please replace the mutation approach with getattr() for safety and cleanliness. The current implementation could cause unexpected side effects.

@DominicOram DominicOram merged commit 182780b into main Nov 12, 2025
21 checks passed
@DominicOram DominicOram deleted the 1122_set_and_wait_should_fail_nicely_on_no_name branch November 12, 2025 17:48
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.

set_and_wait_for_other_value should give the matcher a name if it doesn't have one

4 participants