| 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) *  |
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) * |
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) * |
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) * |
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) * |
Date: 2011-07-27 19:16 |
| Committed. Josh, thanks for the report. |
|
|