[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Changes by Kristján Valur Jónsson :


--
resolution:  -> fixed
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Roundup Robot]

Roundup Robot added the comment:

New changeset ab5e2b0fba15 by Kristján Valur Jónsson in branch '3.3':
The PyCOND_TIMEDWAIT must use microseconds for the timeout argument
http://hg.python.org/cpython/rev/ab5e2b0fba15

New changeset 7764bb7f2983 by Kristján Valur Jónsson in branch '3.4':
Merging from 3.3: The PyCOND_TIMEDWAIT must use microseconds for the timeout 
argument
http://hg.python.org/cpython/rev/7764bb7f2983

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Roundup Robot]

Roundup Robot added the comment:

New changeset 80b76c6fad44 by Kristján Valur Jónsson in branch 'default':
The PyCOND_TIMEDWAIT must use microseconds for the timeout argument
http://hg.python.org/cpython/rev/80b76c6fad44

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

nope, let's not do that :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

I can confirm that the attached test.py times out after 2150 seconds (ie
30+ minutes) with your (tweaked) patch applied:

python test.py 2150

Running Debug|Win32 interpreter...
2014-05-08 10:33:53.670091
Expected to time out by 2014-05-08 11:09:43.670091
Timed Out
2014-05-08 11:09:43.765079

I don't see us adding a test which would lengthen the test suite run by
more than 30 minutes!

--
Added file: http://bugs.python.org/file35184/test.py

___
Python tracker 

___import sys
import threading
import datetime

now = datetime.datetime.now()
print(now)
seconds_to_wait = int(sys.argv[1])
expected_to_finish = now + datetime.timedelta(seconds=seconds_to_wait)
print("Expected to time out by", expected_to_finish)
print("Event Fired" if threading.Event().wait(seconds_to_wait) else "Timed Out")
print(datetime.datetime.now())
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

I see, I wasn't able to compile it yesterday when I did it :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

s/Py_LONG_LONG/PY_LONG_LONG/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Thanks.  Can you confirm that it resolves the issue?  I'll get it checked in 
once I get the regrtest suite run.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

+1 for Kristjan's latest patch. And thanks for working this through, Kristjan: 
I'd missed the fact that the microseconds conversion could itself overflow even 
an unsigned long.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

fix patch, was using git format

--
Added file: http://bugs.python.org/file35176/condwait.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Here is a proposed alternative patch.
No additional checks, just a wider "Py_LONG_LONG us" wide enough to accommodate 
32 bits of milliseconds as before.

--
Added file: http://bugs.python.org/file35175/condwait.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Ah, I saw this code here in thread_nt.h:

if ((DWORD) milliseconds != milliseconds)
Py_FatalError("Timeout too large for a DWORD, "
   "please check PY_TIMEOUT_MAX");

the PyCOND_TIMEDWAIT is currently only used by the GIL code and by the locks on 
NT.  The GIL code assumes microsecond resolution.  So we need to stick to that, 
at least.  But the locking code assumes at least DWORD worth of milliseconds.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

(cont.)
so, I suggest that we modify the API to use "Py_LONG_LONG usec"

Does that sound reasonable?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[R. David Murray]

R. David Murray added the comment:

I'm adding Mark Dickinson to nosy to see if our math expert has a comment on 
the arithmetic :)

--
nosy: +mark.dickinson

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Hi there.
When I said "4000", that was because of the conversion to microseconds which 
happens early on.  

I'm not trying to be difficult here Tim, it's just that you've pointed out a 
problem and I'd like us to have a comprehensive fix.

"unsigned long", I realized, is also not super, because on unix that can be 
either 32 or 64 bits :)

The reason 24 hour waits work on 2.7 is that the conversion to microseconds is 
never done, rather it uses a DWORD of milliseconds.  I agree that this is a 
regression that needs fixing.  Even if there is a theroetical maximum, it 
should be higher than that :)

My latest suggestion?  Let's just go ahead and use a "double" for the argument 
in PyCOND_TIMEDWAIT().

We then have two conversion cases:
1) to a DWORD of milliseconds for both windows apis.  Here we should truncate 
to the max size of a DWORD
2) to the timeval used on pthreads.

for 1, that can be done like:
if (ds*1e3 > (double)DWORD_MAX)
  ms = DWORD_MAX;
else
  ms = (DWORD)(ds*1e3)

for 2, modifying the PyCOND_ADD_MICROSECONDS macro into something like:

#define PyCOND_ADD_MICROSECONDS(tv, ds)
do {
 long oldsec, sec, usec;
 assert(ds >= 0.0);
 // truncate ds into theoretical maximum
 if (ds > (double)long_max)
ds = (double)long_max; // whatever that may be
 sec = (long)ds;
 usec = (long)((ds - (double)sec) * 1e6))
 oldsec = tv.tv_sec;
 tv.tv_usec += usec;
 tv.tv_sec += sec;
 if (usec >= 100) {
   tv.tv_usec -= 100;
   tv.tv_sec += 1;
 }
 if (tv.tv_sec < oldsec)
/* detect overflow */
tv.sec = max_long;

I'm not super experienced with integer arithmetic like this or the pitfalls of 
overflow, so this might need some pondering.  Perhaps it is better to do the 
tv_sec and tv_usec arithmetic in doubles before converting them back.

Does this sound ok?

Let me see if I can cook up an alternative patch.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

Updated patch with unsigned long applied throughout

--
Added file: http://bugs.python.org/file35173/issue20737.condvar.2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

Just to be clear: the change *I'm* proposing for this issue has nothing 
to do with limiting the wait, artificially or otherwise. It's simply 
undoing an unintended conversion from unsigned to signed and back again, 
whicih currently causes any wait of more than 2147 seconds to hang 
pretty much indefinitely.

It looks to me as though Kristjan's "4000" is off by an order of 
magnitude: the parameter to WaitFor... is a DWORD number of 
milliseconds. That allows for 2**31 -1 milliseconds which is something 
short of 50 days, I believe.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[R. David Murray]

R. David Murray added the comment:

I have production code on Windows using python2.7 that calls Event().wait() 
with a timeout of approximately 24 hours, and it works just fine.  Having that 
no longer work is, IMO, an unacceptable regression.  (I'm ready to move this 
code to python3 as soon as cx_Freeze supports Windows services under python3).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

Tim, how about changing the variable to "unsigned long"?  I'd like the 
signature of the function to be the same for all platforms.

This will change the code and allow waits for up to 4000 seconds.

There is still an overflow problem present, though.+

David, in general the maximum wait times of these primitives are platform 
specific.  If you don't want any ceiling, then we would have to add code all 
over the place (in C) to do looping timeouts.  Not sure which is better, to do 
it in c, or to accept in python that waits may timeout earlier than specified.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[R. David Murray]

R. David Murray added the comment:

An implicit ceiling of 4000 seconds on the timeout?  I routinely use timeouts 
of approximately 24 hours in calls to Event().wait().  What am I 
misunderstanding?  If I'm not misunderstanding, then no, I don't think that 
change would be acceptable.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

Thanks for the feedback, Kristjan. You're obviously correct in that we
can't account for timeouts greater than DWORD-size milliseconds and your
proposed solution looks reasonable.

However, I'd like to close off *this* particular issue which turns on
the implicit and, presumably unintended, conversion between unsigned and
signed long in the condition variable code. Can you see any adverse
effect from moving to DWORD parameters per my patch?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Kristján Valur Jónsson]

Kristján Valur Jónsson added the comment:

changing long to DWORD doesn't really fix the overflow issue.
The fundamental problem is that some of the apis, e.g. WaitForSingleObject have 
a DWORD maximum.  so, we cannot support sleep times longer than some particular 
time.

Microseconds was chosen in the api because that is the resolution of the api in 
pthreads.

IMHO, I think it is okay to have an implicit ceiling on the timeout, e.g. some 
4000 seconds.  We can add a caveat somewhere that anyone intending to sleep for 
extended periods of time should be prepared for a timeout occurring early, and 
should have his own timing logic to deal with that.

My suggestion then is to 
a) change the apis to DWORD
b) add a macro something like PyCOND_MAX_WAIT set to 2^32-1
c) properly clip the argument where we call this cunfion, e.g. in lock.acquire.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Changes by Tim Golden :


--
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

The attached patch uses DWORD (essentially: unsigned long) in 
condvar.h:PyCOND_TIMEDWAIT.

Adding Kristjan as it was his code.

--
keywords: +patch
nosy: +kristjan.jonsson
Added file: http://bugs.python.org/file35169/issue20737.condvar.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Tim Golden]

Tim Golden added the comment:

In thread_nt.h:EnterNonRecursiveMutex the millisecond version (here: 2148000) 
of the seconds you passed in are converted to microseconds (so: 214800).

This is then passed to condvar.h:PyCOND_TIMEDWAIT which expects a long, whose 
32-bit limit is 2147483647. So it wraps around and becomes -2146967296. 

That value is then divided by 1000 once again to become milliseconds (-2146967) 
and passed to condvar.h:_PyCOND_WAIT_MS which expects a DWORD, which is a 
synonym for an unsigned long. Thus the signed value becomes an unsigned 
4292820329. Which then passed in as the millisecond timeout to 
WaitForSingleObjectEx.

So that's what's happening; but I'm really not sure at what stage a change 
should be made. It looks to me as if PyCOND_TIMEDWAIT should be accepting 
something more than a long, but I'm really not competent to assess the impact 
of such a change.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Alex Grönholm]

Changes by Alex Grönholm :


--
nosy: +alex.gronholm

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[R. David Murray]

Changes by R. David Murray :


--
nosy: +r.david.murray

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
nosy: +brian.curtin, tim.golden
versions: +Python 3.4

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20737] 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not wake for certain values on Windows

[raruler]

New submission from raruler:

I've tried this with both the 32-bit and 64-bit versions of 3.3.4 on two 
Windows 7 x64 machines. 

threading.Event().wait(2148) and a lock obtained from _thread used as 
lock.acquire(True, 2148) will never return. Anything under 2148 seems to work 
just fine, but anything 2148 or larger never wakes up.

The same call works on 3.3.4/x64 on OS X.

--
components: Library (Lib), Windows
messages: 211966
nosy: pitrou, raruler
priority: normal
severity: normal
status: open
title: 3.3 _thread lock.acquire() timeout and threading.Event().wait() do not 
wake for certain values on Windows
type: behavior
versions: Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com