test_urllib fails in refleak mode

[skrah]

BPO	12923
Nosy	@orsenthil, @meadori, @vadmium
Files	12923-unittest-improvement.patch 12923-maxtries-reset.patch 12923-tries-reset-test.patch 12923-tries-reset.patch: Combined 3 patches

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-02-05.01:57:05.861>
created_at = <Date 2011-09-06.23:40:57.810>
labels = ['tests', 'type-bug', 'library']
title = 'test_urllib fails in refleak mode'
updated_at = <Date 2016-02-05.01:57:05.860>
user = 'https://github.com/skrah'

bugs.python.org fields:

activity = <Date 2016-02-05.01:57:05.860>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-02-05.01:57:05.861>
closer = 'martin.panter'
components = ['Library (Lib)', 'Tests']
creation = <Date 2011-09-06.23:40:57.810>
creator = 'skrah'
dependencies = []
files = ['23358', '23359', '39004', '41302']
hgrepos = []
issue_num = 12923
keywords = ['patch']
message_count = 10.0
messages = ['143656', '145266', '145270', '145271', '149493', '240985', '255946', '256371', '259610', '259616']
nosy_count = 6.0
nosy_names = ['orsenthil', 'meador.inge', 'bbrazil', 'python-dev', 'martin.panter', 'drocco']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12923'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

[skrah]

Hi,

test_urllib fails in refleak mode:

./python -m test -uall -v -R : test_urllib

======================================================================
FAIL: test_invalid_redirect (test.test_urllib.urlopen_HttpTests)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/stefan/hg/cpython/Lib/test/test_urllib.py", line 235, in test_invalid_redirect
    "http://python.org/")
AssertionError: HTTPError not raised by urlopen

---

Ran 58 tests in 0.075s

FAILED (failures=1, skipped=1)
test test_urllib failed
1 test failed:
test_urllib
[133995 refs]

[bbrazil]

This appears to fail every 9th, 19th, 29th, etc. repetition of the test.

This seems to be something to do with the reference counting/close logic of the FakeSocket but I haven't managed to figure out what.

[bbrazil]

The actual problem is that FancyURLOpener self.tries isn't being reset if the protocol is file://

I've attached a patch that'll help improve the test at least.

[bbrazil]

Here's a path to fix the problem.

[meadori]

I just noticed this problem as well.

I don't know the code well enough to determine if Brian's patch is the
right thing to do. The documentation claims that maxtries is used to
put a limit on recursion: http://docs.python.org/dev/library/urllib.request.html#urllib.request.FancyURLopener.
The way the code is written it is limiting recursion, but the recursion count
is *not* being reset when an exception is thrown from 'redirect_internal'.

I see why this is a problem with our test code. 'Lib/test/test_urllib.py'
has the following helper function:

_urlopener = None
def urlopen(url, data=None, proxies=None):
    """urlopen(url [, data]) -> open file-like object"""
    global _urlopener
    if proxies is not None:
        opener = urllib.request.FancyURLopener(proxies=proxies)
    elif not _urlopener:
        opener = urllib.request.FancyURLopener()
        _urlopener = opener
    else:
        opener = _urlopener
    if data is None:
        return opener.open(url)
    else:
        return opener.open(url, data)

Notice that the 'FancyURLopener' instance is cached in a global variable.
The fact that the same instance is used from run to run causes max tries to
be overrun. If resetting maxtries on the exception path isn't safe, then we
can just remove the caching from the tests. The more I think about it, the
more Brian's patch seem correct, though.

Senthil, can you chime in?

[drocco]

Hi,

Here is an alternate patch to the test suite that demonstrates the
failure without needing refleak mode. The test works by issuing enough
requests that, if retries are not independent per request, the test
triggers the code path that results in the test failing. It passes with
the patch to request.py applied.

Thanks,
dan

[vadmium]

All three patches look generally good to me. I left some comments on things I would change.

FTR the tests fail to raise any exception when the redirect limit is reached because FancyURLopener.http_error_default() does not raise an error. It just returns the last result as a synthesized error page.

[vadmium]

I guess this is a real bug so should also be applied to Python 2.

This patch combines the previous three, and tweaks a couple things:

• Rename the test function
• Drop irrelevant header lines

[python-dev]

New changeset eb69070e5382 by Martin Panter in branch '3.5':
Issue bpo-12923: Reset FancyURLopener's redirect counter even on exception
https://hg.python.org/cpython/rev/eb69070e5382

New changeset a8aa7944c5a8 by Martin Panter in branch '2.7':
Issue bpo-12923: Reset FancyURLopener's redirect counter even on exception
https://hg.python.org/cpython/rev/a8aa7944c5a8

New changeset d3be5c4507b4 by Martin Panter in branch 'default':
Issue bpo-12923: Merge FancyURLopener fix from 3.5
https://hg.python.org/cpython/rev/d3be5c4507b4

[vadmium]

One extra change I made to test_redirect_limit_independent() was to stop relying on _urlopener being created before we call urlopen(). As a consequence, in the Python 3 tests I made a wrapper around FancyURLopener to suppress the deprecation warning.