#1499 bind_proxy decorator applied multiple times leads to RecursionError
Closed: Fixed 4 years ago by praiskup. Opened 4 years ago by csomh.

python-copr version:

$ rpm -q python3-copr
python3-copr-1.105-1.fc32.noarch

Reproducer:

import sys
from copr.v3 import Client

sys.setrecursionlimit(64)
for i in range(64):
    Client.create_from_config_file()

output:

Traceback (most recent call last):
  File "test2.py", line 7, in <module>
    Client.create_from_config_file()
  File "/usr/lib/python3.8/site-packages/copr/v3/client.py", line 27, in create_from_config_file
    return cls(config)
  File "/usr/lib/python3.8/site-packages/copr/v3/client.py", line 16, in __init__
    self.project_proxy = ProjectProxy(config)
  File "/usr/lib/python3.8/site-packages/copr/v3/helpers.py", line 59, in wrapper
    result = func(*args, **kwargs)
  File "/usr/lib/python3.8/site-packages/copr/v3/helpers.py", line 59, in wrapper
    result = func(*args, **kwargs)
  File "/usr/lib/python3.8/site-packages/copr/v3/helpers.py", line 59, in wrapper
    result = func(*args, **kwargs)
  [Previous line repeated 54 more times]
  File "/usr/lib/python3.8/site-packages/copr/v3/proxies/__init__.py", line 13, in __init__
    for_all_methods(self.__class__, bind_proxy)
  File "/usr/lib/python3.8/site-packages/copr/v3/helpers.py", line 48, in for_all_methods
    for attr in list(cls.__dict__):
RecursionError: maximum recursion depth exceeded while calling a Python object

I suspect the issue is that bind_proxy is applied in BaseProxy.__init__() on all class methods (probably with the intention so that all methods of all child classes receive this decorator). But as __init__ is called every time a new object is constructed, this will lead the decorator being reapplied each time the proxy objects are constructed, which is: every time a new Client object is created, for example.

If a given Python process creates enough copr Clients, the stack of bind_proxy decorators can become deep enough to exceed the maximum recursion limit of the Python interpreter.

I'm not entirely sure about the solution here, but one fix would be to turn for_all_methods into a class decorator and explicitly decorate all the *Proxy classes with @for_all_methods(bind_proxy) instead of doing it in the constructor of BaseProxy.

(Or probably meta classes could be used, but that would be way to hard to follow and definitely not obvious.)

What do you think? Should I file a PR with the above solution?

The above is the issue we are seeing in Packit and for which a solution was attempted in #1381.


Metadata Update from @praiskup:
- Issue assigned to frostyx

4 years ago

Hello @csomh,
thank you for the detailed report. Very appreciated.

I'm not entirely sure about the solution here, but one fix would be to turn for_all_methods into a > class decorator and explicitly decorate all the *Proxy classes with @for_all_methods(bind_proxy) > instead of doing it in the constructor of BaseProxy.

This is the way I would approach fixing this issue. I wanted to save ourselves from repeating the decorator hence I came with for_all_methods but this magic brought us more issues than it was worth it. Sorry about that. Let's get rid of that and just apply the decorators manually.

Should I file a PR with the above solution?

You already did the hard job of debugging the issue, so the credit is already yours. But if you would like to fix the issue by yourself, you are very welcome. Otherwise, let me know, and I will promptly do it.

Log in to comment on this ticket.

Metadata
Related Pull Requests
  • #1500 Merged 4 years ago