classification
Title: test_urllib fails in refleak mode
Type: behavior Stage: resolved
Components: Library (Lib), Tests Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: bbrazil, drocco, martin.panter, meador.inge, orsenthil, python-dev
Priority: normal Keywords: patch

Created on 2011-09-06 23:40 by skrah, last changed 2016-02-05 01:57 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
12923-unittest-improvement.patch bbrazil, 2011-10-09 16:07 review
12923-maxtries-reset.patch bbrazil, 2011-10-09 16:14 review
12923-tries-reset-test.patch drocco, 2015-04-14 18:40 review
12923-tries-reset.patch martin.panter, 2015-12-14 06:54 Combined 3 patches review
Messages (10)
msg143656 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2011-09-06 23:40
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]
msg145266 - (view) Author: Brian Brazil (bbrazil) * Date: 2011-10-09 15:44
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.
msg145270 - (view) Author: Brian Brazil (bbrazil) * Date: 2011-10-09 16:07
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.
msg145271 - (view) Author: Brian Brazil (bbrazil) * Date: 2011-10-09 16:14
Here's a path to fix the problem.
msg149493 - (view) Author: Meador Inge (meador.inge) * (Python committer) Date: 2011-12-15 03:38
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?
msg240985 - (view) Author: Daniel Rocco (drocco) * Date: 2015-04-14 18:40
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
msg255946 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-05 12:08
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.
msg256371 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-12-14 06:54
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
msg259610 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-05 01:22
New changeset eb69070e5382 by Martin Panter in branch '3.5':
Issue #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 #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 #12923: Merge FancyURLopener fix from 3.5
https://hg.python.org/cpython/rev/d3be5c4507b4
msg259616 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-02-05 01:57
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.
History
Date User Action Args
2016-02-05 01:57:05martin.pantersetstatus: open -> closed
resolution: fixed
messages: + msg259616

stage: patch review -> resolved
2016-02-05 01:22:19python-devsetnosy: + python-dev
messages: + msg259610
2015-12-14 06:54:31martin.pantersetfiles: + 12923-tries-reset.patch

messages: + msg256371
components: + Library (Lib)
versions: + Python 2.7, Python 3.6
2015-12-05 12:08:34martin.pantersetmessages: + msg255946
2015-12-03 19:21:16r.david.murraysetnosy: + martin.panter
2015-04-14 18:40:03droccosetfiles: + 12923-tries-reset-test.patch
versions: + Python 3.5, - Python 3.3
nosy: + drocco

messages: + msg240985
2014-05-13 21:47:28skrahsetnosy: - skrah
2011-12-15 03:38:13meador.ingesetnosy: + meador.inge

messages: + msg149493
stage: patch review
2011-10-09 16:14:01bbrazilsetfiles: + 12923-maxtries-reset.patch

messages: + msg145271
2011-10-09 16:08:00bbrazilsetfiles: + 12923-unittest-improvement.patch
keywords: + patch
messages: + msg145270
2011-10-09 15:44:26bbrazilsetnosy: + bbrazil
messages: + msg145266
2011-09-06 23:40:57skrahcreate