Skip to content
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

Fix propagation of AttributeErrors raised by exposed descriptors #479

Merged
merged 3 commits into from
Jul 2, 2022
Merged

Fix propagation of AttributeErrors raised by exposed descriptors #479

merged 3 commits into from
Jul 2, 2022

Conversation

gschaffner
Copy link
Contributor

Closes #478.

This uses inspect.getattr_static to avoid triggering the descriptor lookup that hasattr usually triggers. The logic also needs a fallback to hasattr since getattr_static "may not be able to retrieve all attributes that getattr can fetch (like dynamically created attributes)".

Per the docs, getattr_static "may find attributes that getattr can’t (like descriptors that raise AttributeError)". I am not aware of any edge cases where hasattr will return False but where getattr_static will succeed without raising an AttributeError (although I did not not dive into CPython's getattr_static implementation to look for such cases), but perhaps there is some edge case that makes using getattr_static a bad choice here. If you know of a better way to fix #478 please let me know :)

I was a bit liberal with adding the tests for multiple configs since the runtime it adds is negligible and extra safety is good. I also slightly broke PEP 8 line length in the tests I added—please change my code style if RPyC uses a different style :)

Fixes bug where an `AttributeError` raised in the `__get__` of an
exposed descriptor `exposed_foo` was only being propagated correctly to
the client if the client accessed the exposed descriptor as
`conn.root.exposed_foo` rather than as `conn.root.foo`.
@gschaffner gschaffner changed the title Exposed descriptor attribute error Fix propagation of AttributeErrors raised by exposed descriptors Feb 27, 2022
@comrumino
Copy link
Collaborator

Sorry for the slow response time!

I think my only gripe would be potential performance implications of try-except blocks. Typically try-except has a higher overhead than if else statement. Do you think it would be possible to change to a if-else logic flow?

@comrumino comrumino self-assigned this Mar 10, 2022
@comrumino comrumino added the Triage Investigation by a maintainer has started label Mar 10, 2022
@gschaffner
Copy link
Contributor Author

Hi, and sorry for the delay!

Considering try-catch overhead makes sense. The try-catch cost can be avoided in the common case if we try-catch on getattr_static() if and only if hasattr(obj, name) is False already—see the latest commit. In cases where hasattr(obj, name) is True (which implies that getattr_static(obj, name) isn't a __get__ descriptor raising AttributeError()), this avoids both (a) try-catch and (b) calling inspect.getattr_static. However, the try-catch + getattr_static overhead would still apply to cases where hasattr() is False. My assumption is that this should be a less common case, though.

As far as I can tell, the only way to rewrite hasattr_static without any try-catch on getattr_static would be to copy the logic of inspect.getattr_static but have it return bools rather than raising. It seems a bit messy to maintain a copy of https://github.com/python/cpython/blob/v3.10.5/Lib/inspect.py#L1731-L1774 though.

I've also rebased onto master to resolve the conflict.

@comrumino comrumino merged commit dc71f0d into tomerfiliba-org:master Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Triage Investigation by a maintainer has started
Projects
None yet
2 participants