urllib.retrieve's reporthook called with non-helpful value

[sidnei]

BPO	1490929
Nosy	@orsenthil, @devdanzin, @4kir4, @iritkatriel
Files	progresshook.diff: Patch adding progresshook test_urllib.patch

^{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/orsenthil'
closed_at = None
created_at = <Date 2006-05-18.13:22:13.000>
labels = ['easy', '3.8', '3.9', '3.10', 'type-feature', 'library']
title = "urllib.retrieve's reporthook called with non-helpful value"
updated_at = <Date 2020-10-26.08:00:19.394>
user = 'https://bugs.python.org/sidnei'

bugs.python.org fields:

activity = <Date 2020-10-26.08:00:19.394>
actor = 'iritkatriel'
assignee = 'orsenthil'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2006-05-18.13:22:13.000>
creator = 'sidnei'
dependencies = []
files = ['12271', '21766']
hgrepos = []
issue_num = 1490929
keywords = ['patch', 'easy']
message_count = 7.0
messages = ['60914', '77257', '81786', '134357', '174870', '379618', '379649']
nosy_count = 7.0
nosy_names = ['orsenthil', 'sidnei', 'ajaksu2', 'zanella', 'nelchael', 'akira', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue1490929'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

• GH-43374: Fix urlretrieve reporthook to report actual bytes read #142653
• [3.14] GH-43374: Fix urlretrieve reporthook to report actual bytes read (GH-142653) #143588
• [3.13] GH-43374: Fix urlretrieve reporthook to report actual bytes read (GH-142653) #143589

[sidnei]

Seems like the 'reporthook', when provided to the
'retrieve' function of urllib will be called with
(blocknumber, blocksize, totalsize).

However, the call to read() might return *less* than
'blocksize' bytes (I believe), so it should imho be
called with len(block) as the second argument instead.
Or maybe add a fourth argument 'readbytes'.

[nelchael]

Attached patch adds new argument: progresshook - it will be passed two
arguments: count of downloaded bytes and total file size (or -1 if it's
not available).

Introducing new argument instead of modifying reporthook maintains
backwards compatibility, also allows removal of reporthook at one point
in the future.

This patch is against r30 SVN tag.

[devdanzin]

Patch has docs, tests needed.

[zanella]

Simple (lazy) test case added.

It just replicates one test case of reporthook to work with progresshook.

The testcases assume the hard-coded value of blocksize on urllib, maybe it should become a public property.

Also commented on diff: http://bugs.python.org/review/1490929/show

[4kir4]

Related bpo-16409

[iritkatriel]

The code in urllib is different now but I see that a test very similar to the one in the patch exists, so was this fixed?

cpython/Lib/test/test_urllib.py

Line 813 in e68c678

def test_reporthook_8193_bytes(self):

[orsenthil]

so was this fixed?

Irit, not really. This is adding another hook called "progress hook" in addition to the "report hook". We need to evaluate if this is really required.