Alexander Belopolsky <belopol...@users.sourceforge.net> added the comment:

The documentation should mention somewhere that timeout can be a float.  For 
example, as in time.sleep docstring:

"""
    sleep(seconds)
    
    Delay execution for a given number of seconds.  The argument may be
    a floating point number for subsecond precision.
"""

I would also like to see some discussion of supported precision.  Is is the 
same as for time.sleep()?  Does float precision ever affect timeout precision? 
(On systems with nanosleep it may, but probably this has no practical 
concequences.)

This can be done as a future enhancement, but I would like to see 
datetime.timedelta as an acceptable type for timeout.  This can be done by 
adding duck-typed code in the error branch which would attempt to call 
timeout.total_seconds() to extract a float.

Looking further, it appears that timeout can be anything that can be added to a 
float to produce float.  Is this an accident of implementation or a design 
decision?  Note that a result Fraction can be used as timeout but Decimal 
cannot.

Zero and negative timeouts are accepted by subprocess.call(), but the result is 
not documented.  It looks like this still starts the process, but kills it 
immediately. An alternative would be to not start the process at all or 
disallow negative or maybe even zero timeouts altogether.  I don't mind the 
current choice, but it should be documented at least in 
Popen.wait(timeout=None) section.

+        def wait(self, timeout=None, endtime=None):
             """Wait for child process to terminate.  Returns returncode
             attribute."""

Docstring should describe timeout and endtime arguments.  In fact I don't see 
endtime documented anywhere.  It is not an obvious choice
that endtime is ignored when timeout is given.  An alternative would be to 
terminate at min(now + timeout, endtime).

+                delay = 0.0005 # 500 us -> initial delay of 1 ms

I think this should be an argument to wait() and the use of busy loop should be 
documented.

+                    delay = min(delay * 2, remaining, .05)

Why .05?  It would probably be an overkill to make this another argument, but 
maybe make it an attribute of Popen, say self._max_busy_loop_delay or a shorter 
descriptive name of your choice.  If you start it with '_', you don't need to 
document it, but users may be able to mess with it if they suspect that 0.05 is 
not the right choice.

+                endtime = time.time() + timeout

Did you consider using datetime module instead of time module here?  (I know, 
you still need time.sleep() later, but you won't need to worry about variable 
precision of time.time().)

----------

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue5673>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to