On Thu, Jul 22, 2010 at 9:05 AM, Alexander Belopolsky
<report@bugs.python.org> wrote:
>
> Alexander Belopolsky <belopolsky@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.)

I added info to wait and communicate, but left the docs for call,
check_call, check_output all saying that their arguments are the same
as Popen(...) and wait(...).

> 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.

I'd prefer to leave it as a future enhancement.

> 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.

Implementation detail.  I don't think it should matter.

> 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).

I didn't intend for the endtime parameter to be documented, it is just
a convenience for implementing communicate, which gets woken up at
various times so it is easier to remember the final deadline rather
than recompute the timeout frequently.

> +                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.

*Points to whoever impelemented it for Thread.wait(timeout=...)*.  If
it was good enough for that (until we got real lock acquisitions with
timeouts), then I think it's good enough for this.

> +                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().)

How does the datetime module help here?  It seems like time.time uses
roughly the same time sources that datetime.datetime.now does.

One other thing I'm worried about here is that time.time can be
non-increasing if the system clock is adjusted.  :(

Maybe someone should file a feature request for a monotonic clock.

Reid