Fix for off-by-one bug in urllib.URLopener.retrieve

[jlmuir]

BPO	810023
Nosy	@birkenfeld, @birkenfeld, @rhettinger
Files	reporthook-HEAD-jlmuir-v1.diff: Patch against HEAD

^{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 = 'https://github.com/birkenfeld'
closed_at = <Date 2005-08-26.08:52:44.000>
created_at = <Date 2003-09-21.05:13:46.000>
labels = ['library']
title = 'Fix for off-by-one bug in urllib.URLopener.retrieve'
updated_at = <Date 2005-08-26.08:52:44.000>
user = 'https://bugs.python.org/jlmuir'

bugs.python.org fields:

activity = <Date 2005-08-26.08:52:44.000>
actor = 'georg.brandl'
assignee = 'georg.brandl'
closed = True
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2003-09-21.05:13:46.000>
creator = 'jlmuir'
dependencies = []
files = ['5599']
hgrepos = []
issue_num = 810023
keywords = ['patch']
message_count = 6.0
messages = ['44669', '44670', '44671', '44672', '44673', '44674']
nosy_count = 5.0
nosy_names = ['georg.brandl', 'georg.brandl', 'rhettinger', 'jlmuir', 'titus']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue810023'
versions = ['Python 2.4']

[jlmuir]

This patch fixes an off-by-one bug in the reporthook part of
urllib.URLopener.retrieve and adds test cases to verify the
fix.

The retrieve method reports reading one more block than it
actually does. Here's an example. The file is empty. I expect
the initial reporthook call on establishment of the network
connection but not an additional call since the file is empty
(no block was read):

>>> import urllib
>>> def reporter(count, blockSize, fileSize):
...     print "c: %i, bs: %i, fs: %i" % (count, blockSize, 
fileSize)
...
>>> srcFile = file("/tmp/empty.txt", "wb")
>>> srcFile.close()
>>> urllib.urlretrieve("file:///tmp/empty.txt", "/tmp/new-
empty.txt", reporter)
c: 0, bs: 8192, fs: 0
c: 1, bs: 8192, fs: 0
('/tmp/new-empty.txt', <mimetools.Message instance at 
0x5a4f58>)
>>>

As a second example, if the file contains 1 byte, the retrieve
method claims it read 2 blocks when it really only read 1:

>>> srcFile = file("/tmp/empty.txt", "wb")
>>> srcFile.write("x")
>>> srcFile.close()
>>> urllib.urlretrieve("file:///tmp/empty.txt", "/tmp/new-
empty.txt", reporter)
c: 0, bs: 8192, fs: 1
c: 1, bs: 8192, fs: 1
c: 2, bs: 8192, fs: 1
('/tmp/new-empty.txt', <mimetools.Message instance at 
0x5a50d0>)
>>>

This patch also includes some changes to
test_urllib.urlretrieve_FileTests (where I added the test
cases) to support creating more than one temporary file and
making sure any created temporary files get deleted when
the test case fixture is torn down.

[titus]

Logged In: YES
user_id=23486

Patch needed slight modification to work against current HEAD; new
patch at

http://issola.caltech.edu/~t/transfer/transfer/patch-810023-
reporthook.diff

The new regression tests look reasonable. I verified that the new
regression tests FAILED against old urllib.py, and worked against patched
urllib.py. Recommend apply.

[rhettinger]

Logged In: YES
user_id=80475

Will review and apply after Xmas vacation.

[rhettinger]

Logged In: YES
user_id=80475

Titus, I'm having difficulty with your link, please attach
the revised patch or just email it to me (python at rcn.com).

[rhettinger]

Logged In: YES
user_id=80475

Reinhold, do you have time for this one?

[birkenfeld]

Logged In: YES
user_id=1188172

Patch looks good, committed as
Lib/urllib.py r1.168 r1.165.2.1
Lib/test/test_urllib.py r1.17 r1.16.4.1