[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: Ok, I've committed the patch to 3.2 and 3.3. I don't really want to bother with 2.7 (it's a rare enough issue). Thank you Chris! -- resolution: -> fixed stage: patch review -> committed/rejected status: open -> closed versions: -Python 2.7 _

[issue15320] thread-safety issue in regrtest.main()

Roundup Robot added the comment: New changeset d7a64e095930 by Antoine Pitrou in branch '3.2': Issue #15320: Make iterating the list of tests thread-safe when running tests in multiprocess mode. http://hg.python.org/cpython/rev/d7a64e095930 New changeset 43ae2a243eca by Antoine Pitrou in branc

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Thanks, Antoine. Just a note: with that other implementation I think the call to pending.close() would probably also warrant a lock since it appears close() can't be called either if a generator is already executing (granted that situation would be even rarer

[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: I would have taken the "simpler" route myself (taking the lock around the next() call), that said it looks ok as it is to me. Do other people have an opinion? -- ___ Python tracker

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Antoine, does the latest patch look okay to you, or did you want something even more minimal (e.g. by defining and acquiring the lock directly in main)? -- ___ Python tracker ___

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Thanks a lot for the feedback. The thinking was to use a stand-alone (even testable) construct with dependencies made explicit. For example, it wasn't obvious that the current iterator depended on forever, or whether the args_tuple parameter was a necessary

[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: I find the patch complicated. If you are using a Lock, surely you don't need a deque, you can keep the original iterator (which is also more readable since it mirrors the semantics quite closely)? -- ___ Python tra

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Here is another patch -- this one making no implementation assumptions about thread-safety or atomicity. -- Added file: http://bugs.python.org/file26371/issue-15320-3.patch ___ Python tracker

[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: > Yes, that is what I took Amaury's comment to mean. I started working on a > patch that incorporates a lock. For the sake of clarity, I think Raymond suggests using a lock in regrtest, not in deque. -- ___ Python

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Yes, that is what I took Amaury's comment to mean. I started working on a patch that incorporates a lock. -- ___ Python tracker ___ _

[issue15320] thread-safety issue in regrtest.main()

Raymond Hettinger added the comment: Why not use locks to guard critical sections rather than relying on implementation details regarding atomicity? -- nosy: +rhettinger ___ Python tracker ___

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: That sounds fine. And thanks for investigating. By the way, I created issue 15329 earlier today to clarify what guarantees deque provides with respect to multithreading. For example, the distinction between thread-safe and atomic is not currently mentioned.

[issue15320] thread-safety issue in regrtest.main()

Amaury Forgeot d'Arc added the comment: > I wrote this patch with the assumption that it shouldn't hurt > if multiple threads call deque.extend() at the same time. By looking at the implementation, I found that if multiple threads call dequeue.extend() at the same time, all items will be added

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Good catch. Here is a patch that takes --forever mode into account. I wrote this patch with the assumption that it shouldn't hurt if multiple threads call deque.extend() at the same time. -- Added file: http://bugs.python.org/file26363/issue-15320-2.

[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: Hmm, actually the patch is not ok, because of the -F option which uses an infinite iterator. -- ___ Python tracker ___ ___

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: Attaching a patch for the original issue using deque. -- Added file: http://bugs.python.org/file26359/issue-15320-1.patch ___ Python tracker __

[issue15320] thread-safety issue in regrtest.main()

Changes by Florent Xicluna : -- nosy: +flox ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: http://mail.python.

[issue15320] thread-safety issue in regrtest.main()

Antoine Pitrou added the comment: Amaury's patch looks obviously fine. As for the original issue: yes, I thought I saw a traceback once due to that. Using list.pop() (or deque.pop()) instead would probably be good enough. -- stage: -> patch review versions: +Python 2.7, Python 3.2 _

[issue15320] thread-safety issue in regrtest.main()

Amaury Forgeot d'Arc added the comment: I was about to say "yes, listiter.__next__ is atomic" (it should, really), but that's not true (Hint: there is a Py_DECREF in the code...). The script below crashes with: Fatal Python error: Objects/listobject.c:2774 object at 0x7f66b7a6c600 has neg

[issue15320] thread-safety issue in regrtest.main()

Chris Jerdonek added the comment: I don't think the late binding is necessary. But it looks like late binding could be preserved simply by constructing args_tuple inside the worker thread instead of in the generator. Really, only "test" needs to be yielded. Nothing else varies in the loop. I

[issue15320] thread-safety issue in regrtest.main()

Amaury Forgeot d'Arc added the comment: There is indeed a race condition here. Fortunately unit tests take much more time than the generator loop. Is it enough to turn the generator into a fixed list? Or is the "late binding" behavior of args_tuple important? (For example, if the main thread

[issue15320] thread-safety issue in regrtest.main()

New submission from Chris Jerdonek : My understanding is that generators are not thread-safe. For example, see http://stackoverflow.com/a/1131458/262819 However, regrtest.main() seems to access a generator from multiple threads when run in multiprocess mode: def work(): # A worker thread