classification
Title: telnetlib.read_until() timeout uses the wrong units
Type: behavior Stage: patch review
Components: Versions: Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Reuben.D'Netto, gregory.p.smith, neologix, orsenthil, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-02-13 05:06 by Reuben.D'Netto, last changed 2013-12-11 19:42 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
telnetlib.py.patch Reuben.D'Netto, 2013-02-13 05:06 Patch
telnetlib.py.patch Reuben.D'Netto, 2013-02-13 05:34
telnetlib.patch Reuben.D'Netto, 2013-02-15 04:12 Final patch review
issue17200.patch serhiy.storchaka, 2013-12-09 19:09 review
Messages (15)
msg181993 - (view) Author: Reuben D'Netto (Reuben.D'Netto) Date: 2013-02-13 05:06
read_until() takes a value for timeout in seconds, but passes it to poll(), which takes a value in milliseconds.
msg181994 - (view) Author: Reuben D'Netto (Reuben.D'Netto) Date: 2013-02-13 05:34
Updated patch to fix expect() as well.
msg181995 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-02-13 05:59
Thanks for the bug report, Reuben. I verified that this is indeed a bug and should be fixed in all versions. Thanks for the patch too, would you like to enhance it with tests? GeneralTests in test_telnetlib.py support timeout and you could that exercise timeout value in secs.
msg181996 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-02-13 06:18
this bug was likely introduced when i applied the telnetlib patches to use poll to not hit the select fd limit.  doh.  nice catch!
msg181997 - (view) Author: Senthil Kumaran (orsenthil) * (Python committer) Date: 2013-02-13 06:39
@gps: looks like it is. For changeset:   78129:de229dde486b for Issue #14635
msg182076 - (view) Author: Reuben D'Netto (Reuben.D'Netto) Date: 2013-02-14 06:11
Sure, no problem. I'll upload the completed patch once I've got it working.
msg182129 - (view) Author: Reuben D'Netto (Reuben.D'Netto) Date: 2013-02-15 04:12
OK, I've implemented tests for read_until() and expect() using both poll and select. I ended up rewriting _read_until_with_select() to look more like the poll equivalent in the process, which should hopefully make it more maintainable.
msg182131 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-02-15 08:12
0 and None must be different.
msg205721 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-09 18:07
The patch is not compatible with 3.4. Does this bug exist in 3.4?
msg205722 - (view) Author: Charles-Fran├žois Natali (neologix) * (Python committer) Date: 2013-12-09 18:11
> The patch is not compatible with 3.4. Does this bug exist in 3.4?

No, selectors all expect a timeout in seconds, so this should be fixed in 3.4.
msg205730 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-09 19:09
Last patch is corrupted and outdated. Here is updated and fixed version. I have not examined it closely.
msg205875 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-12-11 02:09
Review comments added.  I don't really see why the fix should not be as trivial as:

diff -r ca9bca7aecda Lib/telnetlib.py
--- a/Lib/telnetlib.py  Tue Dec 10 16:06:46 2013 -0600
+++ b/Lib/telnetlib.py  Tue Dec 10 18:08:37 2013 -0800
@@ -312,7 +312,9 @@
             poller.register(self, poll_in_or_priority_flags)
             while i < 0 and not self.eof:
                 try:
-                    ready = poller.poll(call_timeout)
+                    # Poll takes its timeout in milliseconds.
+                    ready = poller.poll(None if timeout is None
+                                        else 1000 * call_timeout)
                 except select.error as e:
                     if e.errno == errno.EINTR:
                         if timeout is not None:
@@ -682,7 +684,8 @@
             poller.register(self, poll_in_or_priority_flags)
             while not m and not self.eof:
                 try:
-                    ready = poller.poll(call_timeout)
+                    ready = poller.poll(None if timeout is None
+                                        else 1000 * call_timeout)
                 except select.error as e:
                     if e.errno == errno.EINTR:
                         if timeout is not None:
msg205876 - (view) Author: Roundup Robot (python-dev) Date: 2013-12-11 02:26
New changeset d61e8050b7d7 by Gregory P. Smith in branch '2.7':
Fixes Issue #17200: telnetlib's read_until and expect timeout was broken by the
http://hg.python.org/cpython/rev/d61e8050b7d7

New changeset 46186736e91c by Gregory P. Smith in branch '3.3':
Fixes Issue #17200: telnetlib's read_until and expect timeout was broken by the
http://hg.python.org/cpython/rev/46186736e91c
msg205888 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-12-11 07:18
> Review comments added.

Only original author can answer your questions.

>  I don't really see why the fix should not be as trivial as:

Yes, these are simple and obvious, and only changes which I understand.
msg205917 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-12-11 19:42
anyways, i went with just the simple fix and no specific test for this issue as the tests were painful and questionable reliability.

i appreciate the other refactoring suggestion within the code but for 2.7 and 3.3 bugfixes where no significant changes are ever likely within telnetlib.py they didn't seem warranted.  3.4's code is cleaner thanks to the new selector stuff.
History
Date User Action Args
2013-12-11 19:42:22gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg205917
2013-12-11 07:18:14serhiy.storchakasetmessages: + msg205888
2013-12-11 02:26:26python-devsetnosy: + python-dev
messages: + msg205876
2013-12-11 02:10:31gregory.p.smithsetassignee: gregory.p.smith
2013-12-11 02:09:20gregory.p.smithsetmessages: + msg205875
2013-12-09 19:09:53serhiy.storchakasetfiles: + issue17200.patch

messages: + msg205730
2013-12-09 18:23:00serhiy.storchakasetversions: - Python 3.4
2013-12-09 18:11:55neologixsetmessages: + msg205722
2013-12-09 18:07:01serhiy.storchakasetmessages: + msg205721
stage: test needed -> patch review
2013-12-09 18:05:08serhiy.storchakasetnosy: + neologix

versions: - Python 3.2
2013-02-15 08:12:31serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg182131
2013-02-15 04:12:59Reuben.D'Nettosetfiles: + telnetlib.patch

messages: + msg182129
2013-02-14 06:11:14Reuben.D'Nettosetmessages: + msg182076
2013-02-13 06:39:19orsenthilsetmessages: + msg181997
2013-02-13 06:18:30gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg181996
2013-02-13 05:59:40orsenthilsetversions: + Python 2.7, Python 3.2, Python 3.4
nosy: + orsenthil

messages: + msg181995

stage: test needed
2013-02-13 05:34:27Reuben.D'Nettosetfiles: + telnetlib.py.patch

messages: + msg181994
2013-02-13 05:06:46Reuben.D'Nettocreate