Repository: tomerfiliba-org/rpyc

Issue:
Exposed descriptors raising AttributeErrors cause different remote tracebacks when accessing as `exposed_foo` vs. `foo`
An `AttributeError` raised in the `__get__` of an exposed descriptor `exposed_foo` is only propagated correctly to the client if the client accesses the exposed descriptor as `conn.root.exposed_foo` rather than as `conn.root.foo`. See example below :)

##### Environment

- RPyC version: 5.1.0
- Python version: CPython 3.10.2
- operating system: Manjaro


##### Minimal example

Server:

```python
import rpyc


class Foo:
    def __get__(self, owner, owner_type=None):
        raise AttributeError("This string doesn't always reach the client's traceback!")


class MWEService(rpyc.Service):
    exposed_foo = Foo()


if __name__ == "__main__":
    from rpyc.utils.server import ThreadedServer

    svr = ThreadedServer(MWEService, port=18861)
    svr.start()
```

Client:

```python
import rpyc

conn = rpyc.connect("localhost", port=18861)

# The following two lines will raise different exceptions!
conn.root.exposed_foo
# conn.root.foo
```

##### Tracebacks

When using `conn.root.exposed_foo`:

```python
Traceback (most recent call last):
  File "/tmp/tmp.YPumucnVwE/mwe_c.py", line 6, in <module>
    conn.root.exposed_foo
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/netref.py", line 153, in __getattr__
    return syncreq(self, consts.HANDLE_GETATTR, name)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/netref.py", line 63, in syncreq
    return conn.sync_request(handler, proxy, *args)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 474, in sync_request
    return self.async_request(handler, *args, timeout=timeout).value
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/async_.py", line 103, in value
    raise self._obj
_get_exception_class.<locals>.Derived: This string doesn't always reach the client's traceback!

========= Remote Traceback (1) =========
Traceback (most recent call last):
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 324, in _dispatch_request
    res = self._HANDLERS[handler](self, *args)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 609, in _handle_getattr
    return self._access_attr(obj, name, (), "_rpyc_getattr", "allow_getattr", getattr)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 537, in _access_attr
    return accessor(obj, name, *args)
  File "/tmp/tmp.YPumucnVwE/mwe_s.py", line 6, in __get__
    raise AttributeError("This string doesn't always reach the client's traceback!")
AttributeError: This string doesn't always reach the client's traceback!

```

When using `conn.root.foo`:

```python
Traceback (most recent call last):
  File "/tmp/tmp.YPumucnVwE/mwe_c.py", line 7, in <module>
    conn.root.foo
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/netref.py", line 153, in __getattr__
    return syncreq(self, consts.HANDLE_GETATTR, name)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/netref.py", line 63, in syncreq
    return conn.sync_request(handler, proxy, *args)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 474, in sync_request
    return self.async_request(handler, *args, timeout=timeout).value
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/async_.py", line 103, in value
    raise self._obj
_get_exception_class.<locals>.Derived: cannot access 'foo'

========= Remote Traceback (1) =========
Traceback (most recent call last):
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 324, in _dispatch_request
    res = self._HANDLERS[handler](self, *args)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 609, in _handle_getattr
    return self._access_attr(obj, name, (), "_rpyc_getattr", "allow_getattr", getattr)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 536, in _access_attr
    name = self._check_attr(obj, name, param)
  File "/tmp/tmp.YPumucnVwE/.venv/lib/python3.10/site-packages/rpyc/core/protocol.py", line 526, in _check_attr
    raise AttributeError(f"cannot access {name!r}")
AttributeError: cannot access 'foo'
```

##### Debugging

This appears to occur only when `__get__` raises an `AttributeError` in particular, which makes sense. If `__get__` raises an exception of a different type, it is raised by `hasattr` here:

https://github.com/tomerfiliba-org/rpyc/blob/ce6f432007be8e652ca8480957b2e46599b403d4/rpyc/core/protocol.py#L518

But if `__get__` raises an `AttributeError`, this `hasattr` returns `False` instead of raising, and so the arguments of the `AttributeError` are lost.

Discussion:

Patch:
diff --git a/docs/docs/advanced-debugging.rst b/docs/docs/advanced-debugging.rst
index 92af771..4e685a4 100644
--- a/docs/docs/advanced-debugging.rst
+++ b/docs/docs/advanced-debugging.rst
@@ -20,19 +20,13 @@ Let's use `pyenv` to install Python versions under active development. Since dev
         printf "${PWD}\n" > "${site}/rpyc.pth"
     done
 
-Each `venv` contains a `.pth` file that appends `rpyc` to `sys.path`. We can run `rpyc_classic.py` using `pyenv` like so.
+Each `venv` contains a `.pth` file that appends `rpyc` to `sys.path`.
 
 .. code-block:: bash
 
-    PYENV_VERSION=3.10-dev pyenv exec python ./bin/rpyc_classic.py --host 127.0.0.1
-    PYENV_VERSION=3.9-dev pyenv exec python -c "import rpyc; conn = rpyc.utils.classic.connect('127.0.0.1'); conn.modules.sys.stderr.write('hello world\n')"
+    PYENV_VERSION=3.11-dev pyenv exec python ./bin/rpyc_classic.py --host 127.0.0.1
+    PYENV_VERSION=3.10-dev pyenv exec python -c "import rpyc; conn = rpyc.utils.classic.connect('127.0.0.1'); conn.modules.sys.stderr.write('hello world\n')"
 
-Unit tests can be ran using your desired Python version as well.
-
-.. code-block:: bash
-
-    PYENV_VERSION=3.9-dev pyenv exec python -m unittest discover -s ./tests -k test_affinity
-    PYENV_VERSION=3.8-dev pyenv exec python -m unittest discover -s ./tests
 
 Testing Supported Python Versions via Docker
 --------------------------------------------
@@ -73,7 +67,8 @@ Running the chained-connection unit test
 
 .. code-block:: bash
 
-    python -m unittest discover -s ./tests -k test_get_id_pack.Test_get_id_pack.test_chained_connect
+    cd tests
+    python  -m unittest test_get_id_pack.Test_get_id_pack.test_chained_connect
 
 
 After stopping Wireshark, export specified packets, and open the PCAP. If not already configured, add a custom display column:
diff --git a/rpyc/core/protocol.py b/rpyc/core/protocol.py
index c30bb1f..030f001 100644
--- a/rpyc/core/protocol.py
+++ b/rpyc/core/protocol.py
@@ -6,6 +6,7 @@ import socket
 import time  # noqa: F401
 import gc  # noqa: F401
 
+from inspect import getattr_static
 from threading import Lock, Condition, RLock
 from rpyc.lib import spawn, Timeout, get_methods, get_id_pack
 from rpyc.lib.compat import pickle, next, maxint, select_error, acquire_lock  # noqa: F401
@@ -147,7 +148,7 @@ class Connection(object):
         self._seqcounter = itertools.count()
         self._recvlock = RLock()
         self._sendlock = Lock()
-        self._recv_event = Condition()  # TODO: current use may suggest we could simply timeout and catch an acquire
+        self._recv_event = Condition()
         self._request_callbacks = {}
         self._local_objects = RefCountingColl()
         self._last_traceback = None
@@ -389,24 +390,18 @@ class Connection(object):
                     return False
         # Assume the receive rlock is acquired and incremented
         try:
-            data = None  # Ensure data is initialized
             data = self._channel.poll(timeout) and self._channel.recv()
-        except Exception as exc:
-            if isinstance(exc, EOFError):
-                self.close()  # sends close async request
-            self._recvlock.release()
-            with self._recv_event:
-                self._recv_event.notify_all()
+            if not data:
+                return False
+        except EOFError:
+            self.close()  # sends close async request
             raise
-        else:
+        finally:
             self._recvlock.release()
             with self._recv_event:
                 self._recv_event.notify_all()
-        if not data:
-            return False
-        else:
-            self._dispatch(data)
-            return True
+        self._dispatch(data)
+        return True
 
     def poll(self, timeout=0):  # serving
         """Serves a single transaction, should one arrives in the given
@@ -482,9 +477,13 @@ class Connection(object):
         :returns: the result of the request
         """
         timeout = self._config["sync_request_timeout"]
+        # The recv rlock is acquired prior to invoking the request.
         # AsyncResult will be constructed and it is possible GIL switches
-        # threads before AsyncResult.wait is invoked.
+        # threads before AsyncResult.wait is invoked. So, using an rlock
+        # we acquire once here and once inside of wait which invokes serve
+        self._recvlock.acquire()
         value = self.async_request(handler, *args, timeout=timeout).value
+        self._recvlock.release()
         return value
 
     def _async_request(self, handler, args=(), callback=(lambda a, b: None)):  # serving
@@ -522,6 +521,15 @@ class Connection(object):
         return self._remote_root
 
     def _check_attr(self, obj, name, perm):  # attribute access
+        def hasattr_static(obj, name):
+            try:
+                getattr_static(obj, name)
+            except AttributeError:
+                return False
+            else:
+                return True
+
+
         config = self._config
         if not config[perm]:
             raise AttributeError(f"cannot access {name!r}")
@@ -530,7 +538,7 @@ class Connection(object):
         plain |= config["allow_exposed_attrs"] and name.startswith(prefix)
         plain |= config["allow_safe_attrs"] and name in config["safe_attrs"]
         plain |= config["allow_public_attrs"] and not name.startswith("_")
-        has_exposed = prefix and hasattr(obj, prefix + name)
+        has_exposed = prefix and (hasattr(obj, prefix + name) or hasattr_static(obj, prefix + name))
         if plain and (not has_exposed or hasattr(obj, name)):
             return name
         if has_exposed:
diff --git a/rpyc/core/stream.py b/rpyc/core/stream.py
index 859faaf..952bdec 100644
--- a/rpyc/core/stream.py
+++ b/rpyc/core/stream.py
@@ -12,7 +12,6 @@ from rpyc.core.consts import STREAM_CHUNK
 win32file = safe_import("win32file")
 win32pipe = safe_import("win32pipe")
 win32event = safe_import("win32event")
-ssl = safe_import("ssl")
 
 
 retry_errnos = (errno.EAGAIN, errno.EWOULDBLOCK)
@@ -207,6 +206,7 @@ class SocketStream(Stream):
 
         :returns: a :class:`SocketStream`
         """
+        import ssl
         if kwargs.pop("ipv6", False):
             kwargs["family"] = socket.AF_INET6
         s = cls._connect(host, port, **kwargs)


Test patch:
diff --git a/tests/test_affinity.py b/tests/test_affinity.py
index 684070c..9a208bc 100644
--- a/tests/test_affinity.py
+++ b/tests/test_affinity.py
@@ -43,7 +43,6 @@ class Test_Affinity(unittest.TestCase):
         setattr(rpyc.core.protocol.Connection, '_dispatch', cls._orig_func)
 
     def setUp(self):
-        self._os.sched_setaffinity(0, {0, })
         self.conn = rpyc.connect_thread(rpyc.ClassicService, self.cfg, rpyc.ClassicService, self.cfg)
         self.bg_threads = [rpyc.BgServingThread(self.conn) for i in range(3)]
 
@@ -67,11 +66,20 @@ class Test_Affinity(unittest.TestCase):
         if self._os is not None:
             return self._os.sched_setaffinity(0, self._orig_affinity)
 
+    def test_unpinned(self):
+        """test without changing the default processor affinity"""
+        max_elapsed_time = self.cfg['sync_request_timeout']
+        elapsed_time = self._time_execute_sleep()
+        self.assertLess(elapsed_time, max_elapsed_time)
+        self.assertIn('count=0', repr(self.conn._recvlock))
+
     def test_pinned_to_0(self):
         """test behavior with processor affinity set such that this process is pinned to 0"""
         if self._skip:
             raise self._skip
         max_elapsed_time = self.cfg['sync_request_timeout']
+        self._os.sched_setaffinity(0, {0, })
         elapsed_time = self._time_execute_sleep()
+
         self.assertLess(elapsed_time, max_elapsed_time)
         self.assertIn('count=0', repr(self.conn._recvlock))
diff --git a/tests/test_attr_access.py b/tests/test_attr_access.py
index 60cabed..0d7c160 100644
--- a/tests/test_attr_access.py
+++ b/tests/test_attr_access.py
@@ -74,6 +74,16 @@ class Protector(object):
 SVC_RESTRICTED = ["exposed_foobar", "__add__", "_privy", "foo", "bar"]
 
 
+class MyDescriptor1(object):
+    def __get__(self, instance, owner=None):
+        raise AttributeError("abcd")
+
+
+class MyDescriptor2(object):
+    def __get__(self, instance, owner=None):
+        raise RuntimeError("efgh")
+
+
 class MyService(rpyc.Service):
     exposed_MyClass = MyClass
 
@@ -86,6 +96,10 @@ class MyService(rpyc.Service):
         protector.register(YourClass, ["lala", "baba"])
         return protector.wrap(YourClass())
 
+    exposed_desc_1 = MyDescriptor1()
+
+    exposed_desc_2 = MyDescriptor2()
+
 
 class TestRestricted(unittest.TestCase):
     def setUp(self):
@@ -152,6 +166,11 @@ class TestConfigAllows(unittest.TestCase):
         self.assertRaises(AttributeError, lambda: obj.foo)
         self.assertRaises(AttributeError, lambda: obj.bar)
         self.assertRaises(AttributeError, lambda: obj.spam)
+        root = self.conn.root
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.exposed_desc_1)
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.desc_1)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.exposed_desc_2)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.desc_2)
 
     def test_allow_all(self):
         self._reset_cfg()
@@ -162,6 +181,11 @@ class TestConfigAllows(unittest.TestCase):
         self.assertEqual(obj._privy(), "privy")
         self.assertEqual(obj.foobar(), "Fee Fie Foe Foo")
         self.assertEqual(obj.exposed_foobar(), "Fee Fie Foe Foo")
+        root = self.conn.root
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.exposed_desc_1)
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.desc_1)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.exposed_desc_2)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.desc_2)
 
     def test_allow_exposed(self):
         self._reset_cfg()
@@ -172,6 +196,11 @@ class TestConfigAllows(unittest.TestCase):
         except Exception:
             passed = True
         self.assertEqual(passed, True)
+        root = self.conn.root
+        self.assertRaises(AttributeError, lambda: root.exposed_desc_1)
+        self.assertRaises(AttributeError, lambda: root.desc_1)
+        self.assertRaises(AttributeError, lambda: root.exposed_desc_2)
+        self.assertRaises(AttributeError, lambda: root.desc_2)
 
     def test_allow_safe_attrs(self):
         self._reset_cfg()
@@ -184,6 +213,11 @@ class TestConfigAllows(unittest.TestCase):
         self.assertRaises(AttributeError, lambda: obj.foo)
         self.assertRaises(AttributeError, lambda: obj.bar)
         self.assertRaises(AttributeError, lambda: obj.spam)
+        root = self.conn.root
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.exposed_desc_1)
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.desc_1)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.exposed_desc_2)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.desc_2)
 
     def test_allow_public_attrs(self):
         self._reset_cfg()
@@ -195,6 +229,11 @@ class TestConfigAllows(unittest.TestCase):
         self.assertEqual(obj.foobar(), "Fee Fie Foe Foo")
         self.assertEqual(obj.exposed_foobar(), "Fee Fie Foe Foo")
         self.assertRaises(AttributeError, lambda: obj._privy)
+        root = self.conn.root
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.exposed_desc_1)
+        self.assertRaisesRegex(AttributeError, "abcd", lambda: root.desc_1)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.exposed_desc_2)
+        self.assertRaisesRegex(RuntimeError, "efgh", lambda: root.desc_2)
 
 #    def test_type_protector(self):
 #        obj = self.conn.root.get_two()