[issue4293] Thread Safe Py_AddPendingCall
Mark Dickinson dicki...@gmail.com added the comment: That seems to have done the trick. Thanks! -- status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: checked in r68722, let's hope this fixes things. ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Mark Dickinson dicki...@gmail.com added the comment: Kristján, The r68461 checkin seems to be causing a number of the buildbots to fail, typically with output like: test_capi test test_capi failed -- Traceback (most recent call last): File /home/pybot/buildarea/trunk.klose-debian-ppc/build/Lib/test/test_capi.py, line 57, in test_pendingcalls_threaded self.pendingcalls_wait(l, n) File /home/pybot/buildarea/trunk.klose-debian-ppc/build/Lib/test/test_capi.py, line 42, in pendingcalls_wait timeout waiting for %i callbacks, got %i%(n, len(l))) AssertionError: timeout waiting for 32 callbacks, got 23 Please could you look into this? Also, I don't understand the code: for i in xrange(1000): a = i*i in pendingcalls_wait(), in test_capi.py. Whatever you're trying to do here, surely there's a better way? -- nosy: +marketdickinson status: closed - open ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Mark Dickinson dicki...@gmail.com added the comment: How about using a timer instead of the 'count += 1' loop: after starting the 32 self.pendingcalls_submit threads, set up a threading.Event and start yet another thread that simply does a time.sleep(5.0) (or whatever) and then sets that event. Then your waiting loop could be something like: while not self.my_event.is_set(): for i in range(1000): a = i*i self.assertEqual(len(l), n) There's probably a better way, but that's the best I can come up with this late on a Friday night... ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Checked in as revision: 68460 -- resolution: - accepted status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Antoine Pitrou pit...@free.fr added the comment: Kristján Valur Jónsson krist...@ccpgames.com added the comment: Checked in as revision: 68460 Looks like you forgot the unit tests! ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: I indeed forgot the unittests and docs. They are now in, in revision: 68461 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Thanks. What do you mean by making the test a method of TestCAPI? I have found no such class. Are you talking about the python part or the c part? Also, atomicity in the callback is not required, as the callback is never interrupted implicitly. In fact, that would make it quite useless. I'll add that to the documentation. As for your other suggestions, they're noted, I'll try having more threads. ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Ok, I have addressed the issues raised about testcapi. A new file has been submitted. Added file: http://bugs.python.org/file12619/testcapi.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Antoine Pitrou pit...@free.fr added the comment: Ok, the test patch is fine! Now looking at the main patch, some comments: - you removed volatile from pendingfirst and pendinglast, is it really safe? - what if pending_lock is not initialized when Py_AddPendingCall is called? I know it's unlikely, but imagine someone calling that function just after the interpreter has been initialized - the documentation should mention that the GIL must not be held when Py_AddPendingCall is called, otherwise there can be a deadlock (is it a change from previous versions by the way?) - you should check the return value of PyThread_allocate_lock() for NULL - is your implementation reentrant? that is, registering a callback from a callback. There is no need for it to be, but if it aims to be, then it should perhaps be documented and tested :) ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: 1) volatile is not required when access is guarded by locks. 2) pendingcalls_to_do is initialized to 1, so the very first thing that the interpreter does (after executing one bytecode, I init _PyTicker to 0 now) is to initialize this lock. Creating a lock really must be done by one well known startup thread only. There is a lot of functions in the python API that don't work until after python initialization (without that being explicitly documented), Py_AddPendingCall() is one of them. Btw, there is no generic init function for the ceval.c module. And PyEval_InitThreads isn't really suitable since we must have the lock, even if we haven't initialized the threads, to guard us against reentrancy during signal delivery. For extra safety, I have added a NULL test in Py_AddPendingCall(). 3)Py_AddPendingCall() can be called with our without the GIL. 4)I've added a check in Py_MakePendingCalls(). In PyEval_ReInitThreads there is no way to do anything sensible in the case of error. 5)Yes it is reentrant. The pending_lock is only held for a short while while we pop a callback off the queue. However, because even during that short while a signal can be delivered which causes Py_AddPendingCall() to be called, the latter function fails if it doesn't acquire the lock in a reasonable amount of time. This would cause a signal to be lost, and not a deadlock. This hasn't changed from the old implementation. I have also added a flag to make sure that we don't execute pending calls recursively, (async notifications is so much better) and explained it better in the docs. Added file: http://bugs.python.org/file12624/pendingalls.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Amaury Forgeot d'Arc amaur...@gmail.com added the comment: - The things_to_do static variable should be declared as volatile: it is read by the main loop without any lock. (by the way, could you rename it to something like pendingcalls_to_do?) - in the old Py_MakePendingCalls function, the #ifdef WITH_THREAD part is not useful now. - the XXX comments should probably be rewritten. A note to people concerned by performance: the additional lock is used only when there is an actual pending call to process. The main loop only regularly checks the things_to_do static volatile static variable, and the patch does not change this. ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Good points. Any suggestion where to document the new functionality? ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Ah, I forgot the test code in my patch. I submit a separate patch to add that code. Also, reworking things to have single function definitions is in my opinion too messy. I started doing things that way, but the code becomes hard to read and therefore to understand and verify. Added file: http://bugs.python.org/file12596/testcapi.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Antoine Pitrou pit...@free.fr added the comment: A few comments about the test: - it should be a method of TestCAPI; we try to unittest everywhere, although there is some old code which predates that - the implementation would be stressed better if each callback was added from a separate thread, rather than adding them all at once; it will also make the C function in _testcapi simpler (and you can do the sleep in the Python code in test_capi.py) - if you want the list operation in your callback to be atomic, it should append() something to the list rather than increment its first element; then you just have to test for the len() of the list - in _pending_callback(), you must use Py_XDECREF(r), not Py_DECREF, since r can be NULL I will look at the ceval part of the patch later :) ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Amaury Forgeot d'Arc amaur...@gmail.com added the comment: Well, I could not find these functions documented anywhere :-( If you want to start to write some, a good place could be in http://docs.python.org/c-api/init.html (Doc/c-api/init.rst, maybe after the PyGILState_ functions) At the very least, a short sentence in whatsnew/2.7.rst would be appreciated. ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson krist...@ccpgames.com added the comment: Ping. Amaury, any thoughts? ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: Small refinement: Deadlock avoidance is only needed on the main thread since we only install signal handlers for the main thread. Added file: http://bugs.python.org/file12031/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment: Two comments about the patch: - typo in a comment: ...change chat a signal... - PyThread_acquire_lock should not be called with a NULL lock: on win32 this does not matter (there is a test in thread_nt.h), but Unix segfaults in thread_pthread.h if you hit Ctrl-C. ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: I had forgotten that locks aren't initialized until PyEval_InitThreads () is called. Now we check for this case. Also reinitialize when fork () is called. Is there any suggested place where I should add documentation to this? Added file: http://bugs.python.org/file12032/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: I turns out that for SIGINT, windows creates a new thread. So, the assumption that signals are delivered on the main thread are not valid in general. Therefore, we must initialize the lock pending_lock independently of PyEval_InitThreads(), and also cannot assume that signals are deliverd on the main thread. Added file: http://bugs.python.org/file12035/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: Here is a revised version. Since there is no portable way to block signals, and no way at all on windows (apparently) the simplest way is to simply use NOWAIT_LOCK when adding a new pending call. While this does not guarantee that we are always able to append a call (we may be unlucky in all our N tries and there is also no portable way to yield the thread timeslice between tries) such are also the semantics of the method. Sometimes the buffer may be full, or we fail to get the lock in time, and a -1 is returned. It is up to the caller to respond appropriately, perhaps by trying again later. Added file: http://bugs.python.org/file12008/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: Right. Isn't the best way to avoid it to block signals just while we pop the queue? I'll see if python has a portable sigaction thing to do that. Otherwise we'd have to start to mess with a volatile busy flag again and possibly introduce tricky race conditions. ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Amaury Forgeot d'Arc [EMAIL PROTECTED] added the comment: Py_AddPendingCall is used inside signal handlers. It seems that there is a (tiny) chance that a signal interrupts the program inside Py_MakePendingCalls, while the pendinglock is held. Py_AddPendingCall would try to acquire this lock again and deadlock. -- nosy: +amaury.forgeotdarc ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Changes by Kristján Valur Jónsson [EMAIL PROTECTED]: Removed file: http://bugs.python.org/file11974/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
New submission from Kristján Valur Jónsson [EMAIL PROTECTED]: At CCP We have started using the Py_AddPendingCall() mechanism to signal python about a completed IO operation. However, we noticed that the existing mechanism was hoplelessly un- thread safe. This is bad, since on Windows at least, it is very convenient to have such callbacks happen on an arbitrary thread from the system's thread pool. I submit a thread-safe implementation instead to be used if WITH_THREAD is defined. This allows Py_AddPendingCall() to be called from any thread, from any context, even from a PendingCall callback itself. -- components: Interpreter Core files: pendingalls.patch keywords: needs review, patch, patch messages: 75691 nosy: krisvale priority: normal severity: normal status: open title: Thread Safe Py_AddPendingCall type: crash versions: Python 2.7 Added file: http://bugs.python.org/file11974/pendingalls.patch ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Kristján Valur Jónsson [EMAIL PROTECTED] added the comment: Good point. I'll make a test using ctypes and _testcapimodule.c to try to make it as non-platform-specific as possible. ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue4293] Thread Safe Py_AddPendingCall
Christian Heimes [EMAIL PROTECTED] added the comment: Your patch sounds like a useful addition. However I can comment on the details because I haven't dealt with Py_AddPendingCall() yet. Documentation updates and a unit test (Modules/_testcapimodule.c), that shows the bug and verifies your patch, are welcome, too. -- keywords: -needs review nosy: +christian.heimes stage: - patch review versions: +Python 3.1 ___ Python tracker [EMAIL PROTECTED] http://bugs.python.org/issue4293 ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
