ionelmc / python-hunter

Hunter is a flexible code tracing toolkit.
https://python-hunter.readthedocs.io/
BSD 2-Clause "Simplified" License
796 stars 46 forks source link

Calling repr() on objects may affect execution flow #67

Closed borman closed 5 years ago

borman commented 5 years ago

Here is a reproducible example: https://gist.github.com/borman/f4b347e87ee3a528c6ac94285084293b

You can see a diff in the execution trace between two runs. Execution flow diverges after client_reqrep.py:875: https://gist.github.com/borman/f4b347e87ee3a528c6ac94285084293b/revisions?diff=unified#diff-3c628d58096772043814ea5c7b40f0b0

The reason for this behaviour is:

  1. aiohttp authors implemented cached properties (thus reading their value actually mutates the object): https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client_reqrep.py#L685-L687 https://github.com/aio-libs/aiohttp/blob/master/aiohttp/helpers.py#L368-L396
  2. repr() implementation uses these properties: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client_reqrep.py#L741
  3. the code was written with an assumption that these properties will not be accessed before being initialized
  4. hunter breaks this assumption by calling repr() early after object creation (what is seen in trace as a "failed repr")
  5. some properties (e.g. headers) are cached in their uninitialized state
ionelmc commented 5 years ago

There is indeed a side-effect but hunter doesn't use repr by default, it uses https://github.com/ionelmc/python-hunter/blob/master/src/hunter/util.py#L150

borman commented 5 years ago

Unfortunately, it doesn't help. I only used native repr() for an easy implementation: as you can verify, this example fails with default safe_repr as well.

ionelmc commented 5 years ago

It appears that methods are unsafe to repr (they call repr on the instance). I'll fix this.

Eg:

>>> class Junk:
...   def __repr__(self):
...     return "Bad Stuff"
...   def foo(self): pass
...
>>>
>>> Junk().foo
<bound method Junk.foo of Bad Stuff>
ionelmc commented 5 years ago

@borman Give the master a try?