avoid infinite regress in getattr #41

bukzor · 2014-09-16T22:59:16Z

After a large amount of debugging a deadlock in pip's test suite, I found an infinite recursion in the below __getattr__ method. I've fixed it and regression-tested it as best I could.

It's not clear to me how an attribute of this class is referenced after its __fp is deleted, but it did happen.

For historicity's sake, this is the stack that caused the inifinite recursion:

  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/workspace/venv/bin/pip", line 9, in <module>
    load_entry_point('pip==1.6.dev1', 'console_scripts', 'pip')()
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/__init__.py", line 198, in main
    return command.main(cmd_args)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/basecommand.py", line 212, in main
    status = self.run(options, args)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/commands/install.py", line 317, in run
    requirement_set.prepare_files(finder)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/req/req_set.py", line 238, in prepare_files
    req_to_install, self.upgrade)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 284, in find_requirement
    for page in self._get_pages(url_locations, req):
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 391, in _get_pages
    page = self._get_page(location, req)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 611, in _get_page
    result = HTMLPage.get_page(link, req, session=self.session)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 693, in get_page
    "Cache-Control": "max-age=600",
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 463, in get
    return self.request('GET', url, **kwargs)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/download.py", line 286, in request
    result = super(PipSession, self).request(method, url, *args, **kwargs)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 451, in request
    resp = self.send(prep, **send_kwargs)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 557, in send
    r = adapter.send(request, **kwargs)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/adapter.py", line 38, in send
    return self.build_response(request, cached_response, from_cache=True)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/adapter.py", line 95, in build_response
    request, response
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/adapters.py", line 203, in build_response
    response.headers = CaseInsensitiveDict(getattr(resp, 'headers', {}))
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/structures.py", line 46, in __init__
    self.update(data, **kwargs)
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/workspace/venv/lib/python2.7/_abcoll.py", line 541, in update
    for key in other:
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/packages/urllib3/response.py", line 304, in closed
    elif hasattr(self._fp, 'isclosed'):  # Python 2
  File "/tmp/pytest-29/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrapper.py", line 27, in __getattr__
    f.write(''.join(traceback.format_stack()))

I believe that gc is activating during Mapping.update, and jumping to response.closed. Why it would touch a property, I don't know. Further, this is happening after something (the gc I assume) has deleted __fp from the filewrapper. Because filewrapper references self.__fp in __getattr__, and __fp isn't present, it recurses there. Why I don't get a StackOverflowError rather than a busy deadlock, I also don't know.

But, this fixes it :D

bukzor · 2014-09-16T23:01:35Z

Without the patch, the new test fails like so:

________________________________ test_getattr_during_gc ________________________________

    def test_getattr_during_gc():
        s = CallbackFileWrapper(None, None)
        # normal behavior:
        with pytest.raises(AttributeError):
            s.x

        # this previously had caused an infinite recursion
        del s._CallbackFileWrapper__fp
        with pytest.raises(AttributeError):
>           s.x

tests/test_regressions.py:31: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cachecontrol/filewrapper.py:22: in __getattr__
    return getattr(self.__fp, name)
cachecontrol/filewrapper.py:22: in __getattr__
    return getattr(self.__fp, name)
cachecontrol/filewrapper.py:22: in __getattr__
    return getattr(self.__fp, name)
!!! Recursion detected (same locals & position)
==================== 1 failed, 36 passed, 1 skipped in 0.57 seconds ====================

dstufft · 2014-09-16T23:05:31Z

This is really confusing to me... I feel like maybe we're missing something here. My guess is it's somehow getting into a never ending loop when the responses close() method is being called by the responses __del__.

Is the munged name actually guaranteed to be stable across versions and Python implementations?

dstufft · 2014-09-16T23:08:58Z

What happens if instead you do:

class CallbackFileWrapper(object):

    # ...

    def __del__(self):
        del self.__calback
        del self.__fp
        del self.__buf

does that work?

sigmavirus24 · 2014-09-16T23:10:28Z

Is the munged name actually guaranteed to be stable across versions and Python implementations?

Was my concern as well.

bukzor · 2014-09-16T23:14:20Z

The exact name-mangling behavior is documented in the language reference: https://docs.python.org/2/reference/expressions.html#atom-identifiers

I would very much expect it to be the same cross-implementation.

We can also un-mangle this name if it makes everyone happier.

bukzor · 2014-09-16T23:25:28Z

A printout from faulthandler of the same scenario:

Timeout (0:00:10)!
Current thread 0x00007f6b60150740:
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
<snip>
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/filewrappe...", line 22 in __getattr__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/packages/urlli...", line 304 in closed
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/workspace/venv/lib/python2.7/_abcoll.py", line 541 in update
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/structures.py", line 46 in __init__
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/adapters.py", line 203 in build_response
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/adapter.py", line 95 in build_response
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/cachecontrol/adapter.py", line 38 in send
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 557 in send
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 451 in request
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/download.py", line 286 in request
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/_vendor/requests/sessions.py", line 463 in get
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 693 in get_page
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 611 in _get_page
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 391 in _get_pages
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/index.py", line 284 in find_requirement
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/req/req_set.py", line 238 in prepare_files
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/commands/install.py", line 317 in run
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/basecommand.py", line 212 in main
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/pip_src/pip/__init__.py", line 198 in main
  File "/tmp/pytest-33/test_upgrade_user_conflict_in_globalsite0/workspace/venv/bin/pip", line 9 in <module>

alex · 2014-09-16T23:58:50Z

This patch looks reasonable to me -- I don't know if the name manglign is guarnteed, but like 500000 things rely on it, so I wouldn't worry in the least.

dstufft · 2014-09-17T00:00:56Z

cachecontrol/filewrapper.py

+        # The vaguaries of garbage collection means that self.__fp is not always set.
+        # This strange style lets us throw AttributeError in that case, rather than
+        # recursing infinitely
+        fp = object.__getattribute__(self, '_CallbackFileWrapper__fp')


As said in IRC, I think checking explicitly for the attribute name in the __getattr__ is cleaner, although if this style is kept, does self.__getattribute__("_CallbackFileWrapper__fp") not work?

dstufft · 2014-09-17T00:01:28Z

If @alex thinks relying on the name mangling is safe then it's probably safe.

dstufft · 2014-09-17T00:07:41Z

This looks reasonable enough to me!

sigmavirus24 · 2014-09-17T00:10:26Z

The name mangling still irks me but if it's safe, that's cool. LGTM

bukzor · 2014-09-17T00:20:14Z

I believe we have consensus.

sigmavirus24 · 2014-09-17T01:00:02Z

Consensus does not a pull request merge.

ionrock · 2014-09-17T02:19:20Z

@bukzor I'm happy to merge this, but would it be too much to ask to get a little more clarification in the comment? Specifically it would be helpful to get short summary of what happened, why this fixes it and why the _CallbackFileWrapper__fp is an appropriate bit of name munging. I'm making an assumption that is the namemunging @dstufft mentioned.

I hope this doesn't come across as "nit picky". My real reason is that I'm not terribly sure I totally understand the fix, which means if I look at it again in a few weeks my lack of understanding will undoubtedly multiplied a few times over! Feel free to leave a comment here if that is easier and I'll update the comment myself.

Sorry about difficult time debugging this and thanks for the PR!

avoid infinite regress in __getattr__

ionrock · 2014-09-17T02:20:58Z

@bukzor Nevermind! I found where I was confused. Thanks again!

sigmavirus24 · 2014-09-17T02:28:48Z

@ionrock if I were you, I'd add comments in case future you becomes confused again.

bukzor · 2014-09-17T03:02:36Z

@iorock 👍
Thanks for the merge!
I always make my comments too short =/

ionrock · 2014-09-17T15:28:10Z

FYI, this has been released in 0.10.4.

bukzor force-pushed the buck-less-deadlock branch from fdf7518 to aeb40ea Compare September 16, 2014 23:56

bukzor force-pushed the buck-less-deadlock branch from aeb40ea to fa0debe Compare September 16, 2014 23:59

dstufft reviewed Sep 17, 2014
View reviewed changes

avoid infinite regress in __getattr__

7534ba4

bukzor force-pushed the buck-less-deadlock branch from fa0debe to 7534ba4 Compare September 17, 2014 00:06

ionrock added a commit that referenced this pull request Sep 17, 2014

Merge pull request #41 from bukzor/buck-less-deadlock

042ae8c

avoid infinite regress in __getattr__

ionrock merged commit 042ae8c into psf:master Sep 17, 2014

bukzor deleted the buck-less-deadlock branch September 17, 2014 03:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid infinite regress in getattr #41

avoid infinite regress in getattr #41

bukzor commented Sep 16, 2014

bukzor commented Sep 16, 2014

dstufft commented Sep 16, 2014

dstufft commented Sep 16, 2014

sigmavirus24 commented Sep 16, 2014

bukzor commented Sep 16, 2014

bukzor commented Sep 16, 2014

alex commented Sep 16, 2014

dstufft Sep 17, 2014

dstufft commented Sep 17, 2014

dstufft commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

bukzor commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

ionrock commented Sep 17, 2014

ionrock commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

bukzor commented Sep 17, 2014

ionrock commented Sep 17, 2014

avoid infinite regress in __getattr__ #41

avoid infinite regress in __getattr__ #41

Conversation

bukzor commented Sep 16, 2014

bukzor commented Sep 16, 2014

dstufft commented Sep 16, 2014

dstufft commented Sep 16, 2014

sigmavirus24 commented Sep 16, 2014

bukzor commented Sep 16, 2014

bukzor commented Sep 16, 2014

alex commented Sep 16, 2014

dstufft Sep 17, 2014

Choose a reason for hiding this comment

dstufft commented Sep 17, 2014

dstufft commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

bukzor commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

ionrock commented Sep 17, 2014

ionrock commented Sep 17, 2014

sigmavirus24 commented Sep 17, 2014

bukzor commented Sep 17, 2014

ionrock commented Sep 17, 2014

avoid infinite regress in getattr #41

avoid infinite regress in getattr #41