classification
Title: pydoc.synopsis breaks if filesystem returns mtime of 0
Type: Stage: resolved
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, haypo, joshtriplett, ncoghlan, neologix, python-dev, ron_adam
Priority: normal Keywords: needs review, patch

Created on 2011-07-21 20:33 by joshtriplett, last changed 2011-07-27 19:17 by neologix. This issue is now closed.

Files
File name Uploaded Description Edit
pydoc_mtime.diff neologix, 2011-07-23 17:21 review
Messages (10)
msg140826 - (view) Author: Josh Triplett (joshtriplett) Date: 2011-07-21 20:33
In Python 2.7.2, pydoc.py's synopsis contains this code implementing a cache:

    mtime = os.stat(filename).st_mtime
    lastupdate, result = cache.get(filename, (0, None))
    if lastupdate < mtime:

Many filesystems don't have any concept of mtime or don't have it available, including many FUSE filesystems, as well as our implementation of stat for GRUB in BITS.  Such systems typically return an mtime of 0.  (In addition, 0 represents a valid mtime.)  Since the cache in pydoc.synopsis initializes lastupdate to 0 for entries not found in the cache, this causes synopsis to always return None.  I'd suggest either extending the conditional to check "lastupdate != 0 and lastupdate < mtime" (which would always treat an mtime of 0 as requiring an update, which would make sense for filesystems without valid mtimes) or changing the .get to return (None, None) and checking "lastupdate is not None and lastupdate < mtime", which would treat an mtime of 0 as valid but still handle the case of not having a cache entry the first time.
msg140910 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-22 21:33
Hi!  Thanks for the report and patch ideas.  Would both of your fix ideas preserve backward compatibility?  If yes, we should take the one that makes the code easier to read; if no, we should take the most compatible.  Would you be interested in making a patch?  If so, helpful guidelines are available under http://docs.python.org/devguide; if not, someone else will do it.
msg140922 - (view) Author: Josh Triplett (joshtriplett) Date: 2011-07-23 01:09
The current behavior of pydoc will cause synopsis to always incorrectly return "None" as the synopsis for any module with mtime == 0.  Both of the proposed fixes will fix that bug without affecting any case where mtime != 0, so I don't think either one has backward-compatibility issues.

I'd suggest using the fix of changing the .get call to return a default of (None, None) and changing the conditional to "lastupdate is not None and lastupdate < mtime".  That variant seems like more obvious code (since None clearly means "no lastupdate time"), and it avoids special-casing an mtime of 0 and bypassing the synopsis cache.

I don't mind writing a patch if that would help this fix get in.  I'll try to write onein the near future, but I certainly won't mind if someone else beats me to it. :)
msg140997 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-23 17:21
Demo:

"""
cf@neobox:~/cpython$ ./python -c "import pydoc; print(pydoc.synopsis('Lib/os.py'))"
OS routines for Mac, NT, or Posix depending on what system we're on.
[51835 refs]
cf@neobox:~/cpython$ touch -t 197001010000 Lib/os.py 
cf@neobox:~/cpython$ ./python -c "import pydoc; print(pydoc.synopsis('Lib/os.py'))"
None
[51833 refs]
"""

> I'd suggest using the fix of changing the .get call to return a default of (None,
> None) and changing the conditional to "lastupdate is not None and
> lastupdate < mtime".

You mean "lastupdate is None or lastupdate < mtime"? Otherwise, a file not present in the cache would never be looked-up.

Here's a patch.
It's obvious, but note that if the filesystem doesn't provide mtime, then once the metadata has been cached, it won't be refreshed if the file is updated.

I'll take a look around the standard library for similar issues.

Note: it would of course be simpler to use -1 as the default mtime value, but a negative time_t is possible.
msg141021 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-23 21:46
Patch looks good.  Does this require a doc update, or is it entirely an internal function?
msg141024 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-23 23:02
Well, the function is part of pydoc's public API, but the inner
working of the cache mechanism is completely private: this won't have
any impact, other than fixing the bug :-)
msg141250 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-27 17:36
New changeset 2229b0422369 by Charles-François Natali in branch '2.7':
- Issue #12603: Fix pydoc.synopsis() on files with non-negative st_mtime.
http://hg.python.org/cpython/rev/2229b0422369
msg141251 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-27 17:39
New changeset 2bc740a83010 by Charles-François Natali in branch '3.2':
Issue #12603: Fix pydoc.synopsis() on files with non-negative st_mtime.
http://hg.python.org/cpython/rev/2bc740a83010
msg141252 - (view) Author: Roundup Robot (python-dev) Date: 2011-07-27 17:41
New changeset 5f003d725619 by Charles-François Natali in branch 'default':
Issue #12603: Fix pydoc.synopsis() on files with non-negative st_mtime.
http://hg.python.org/cpython/rev/5f003d725619
msg141268 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-07-27 19:16
Committed.
Josh, thanks for the report.
History
Date User Action Args
2011-07-27 19:17:46neologixsetstatus: open -> closed
2011-07-27 19:16:30neologixsetresolution: fixed
messages: + msg141268
stage: patch review -> resolved
2011-07-27 17:41:30python-devsetmessages: + msg141252
2011-07-27 17:39:48python-devsetmessages: + msg141251
2011-07-27 17:36:37python-devsetnosy: + python-dev
messages: + msg141250
2011-07-23 23:02:35neologixsetmessages: + msg141024
2011-07-23 21:46:56eric.araujosetmessages: + msg141021
2011-07-23 17:21:36neologixsetkeywords: + patch, needs review
files: + pydoc_mtime.diff
messages: + msg140997

stage: needs patch -> patch review
2011-07-23 16:02:19pitrousetnosy: + neologix
2011-07-23 01:09:49joshtriplettsetmessages: + msg140922
2011-07-22 21:33:30eric.araujosetnosy: + ncoghlan, haypo, ron_adam, eric.araujo
title: pydoc.synopsis breaks if filesystem returns mtime of 0 (common for filesystems without mtime) -> pydoc.synopsis breaks if filesystem returns mtime of 0
messages: + msg140910

versions: + Python 3.2, Python 3.3
stage: needs patch
2011-07-21 20:33:11joshtriplettcreate