[issue22926] asyncio: raise an exception when called from the wrong thread
Roundup Robot added the comment: New changeset 4e8ac4173b3c by Victor Stinner in branch '3.4': Issue #22926: In debug mode, call_soon(), call_at() and call_later() methods of https://hg.python.org/cpython/rev/4e8ac4173b3c -- nosy: +python-dev ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Thanks for the review. I commited my change in Tulip, Python 3.4 3.5. -- resolution: - fixed status: open - closed ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: I'm going to commit check_thread-3.patch if nobody complains: in debug mode, call_soon(), call_at() and call_later() now check threading.get_ident() to ensure that they are called from the thread running the event loop. -- Added file: http://bugs.python.org/file37542/check_thread-3.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: SGTM. Merry Xmas! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Rebased patch which now always check the thread, even in release mode. Summary of the current version of the patch (check_thread-2.patch): - call_soon/call_at now checks if they are called from the thread running the event loop - the check is only done when the event loop in running - the check is always done, in relase and debug mode (we may only do it in release mode if you consider that the overhead is too high, see benchmark numbers below) - add a unit test for the thread check - since the check is only done when the event loop in running, we don't need the hack to avoid the thread check in the proactor event loop, for self._call_soon(self._loop_self_reading, ()) - replace the _running attribute with a new _owner attribute in BaseEventLoop Victor, can you benchmark this? Here is a benchmark for call_soon(): bench_call_soon.py. It computes the performance of one call to call_soon() in average. It uses 10,000 iterations, the test is run 5 times and the minimum timing is displayed. Since a call to traceback.extract_stack() takes more than 65 us, whereas a call to call_soon() in release mode takes 2.8 us, I removed the call in Handle constructor for the benchmark to focus on the thread check. - asyncio without extract_stack(), in release mode: 2491 ns per call - asyncio without extract_stack(), in debug mode: 3588 ns per call (1.4x slower, +1097 ns) - asyncio without extract_stack(), with check_thread-2.patch: 2721 ns per call (1.1x slower, +230 ns) The overhead of check_thread-2.patch is +230 ns (1.1x slower) per call to call_soon(). Do you consider that the overhead is low enough to run the check even in release mode? At least on Linux, threading.get_ident() is not a system call. Performances may be different on other platforms (ex: BSD, Windows). In check_thread-2.patch, I inlined the thread check directly in call_at() and call_soon(). For performance, but also because the check only takes 2 lines, and so the error message can contain the function name (call_soon/call_at). -- Added file: http://bugs.python.org/file37518/check_thread-2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Changes by STINNER Victor victor.stin...@gmail.com: Added file: http://bugs.python.org/file37519/bench_call_soon.py ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Guido So in order to do correct diagnostics here we would have to add an owning thread pointer to every event loop; I didn't understand why this approach was not taken when the check was introduced. I was surprised that get_event_loop() was used for the check instead. Here is a patch checking the thread using threading.get_ident(). The check still works when set_event_loop(None) is used in unit tests. I created the issue #23074 for the change: get_event_loop() must always raise an exception, even when assertions are disabled by -O. -- asyncio requires thread support, it is already the case without my patch. For example, the event loop policy uses threading.Local. In the aioeventlet project, call_soon() writes a byte in the self pipe (to wake up the selector) if the selector is waiting for events; call_soon() is green thread safe. It is less efficient than calling forcing users to call explicitly call_soon_threadsafe(), but aioeventlet is written to be compatible with asyncio and eventlet, while these projects are very different. In the aiogevent and aioeventlet projects, I also store the current greenlet, but for a different purpose: ensure that yield_future() is not called in the greenlet running the event loop, which would blocking. -- Added file: http://bugs.python.org/file37483/check_thread.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Guido wrote: I'm okay with this approach now. I'm not sure that I understood your opinion. Are you ok to raise an exception in debug mode if call_soon() is called from a thread which has no event loop attached? (Currently, an exception is already raised in debug mode, but only if the thread has an attached event loop, but not the good one.) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: Well, the PEP clearly states that get_event_loop() should never return None and raise an exception if the context has no environment. (Where context ~~ thread.) On Tue, Dec 16, 2014 at 3:48 PM, STINNER Victor rep...@bugs.python.org wrote: STINNER Victor added the comment: Guido wrote: I'm okay with this approach now. I'm not sure that I understood your opinion. Are you ok to raise an exception in debug mode if call_soon() is called from a thread which has no event loop attached? (Currently, an exception is already raised in debug mode, but only if the thread has an attached event loop, but not the good one.) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: 2014-12-17 0:53 GMT+01:00 Guido van Rossum rep...@bugs.python.org: Well, the PEP clearly states that get_event_loop() should never return None and raise an exception if the context has no environment. (Where context ~~ thread.) You don't reply to my question, or I misunderstood your anwer. call_soon() is a method an event loop, there is no need to call get_event_loop(). get_event_loop() is only called in debug mode to help the developer to detect obvious bugs. I don't think that the behaviour of the debug mode must be written in the PEP, it's more implementation specific. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: Oh sorry. I'm in too much of a hurry. :-( The problem is that the thread may not have a loop, but the loop still has a tread. E.g. the tests intentionally call set_event_loop(None) to make sure that no code depends on the implict event loop. An app should be allowed to do the same, even in debug mode. IIUC this is already broken in debug mode? (Or perhaps only in non-main threads?) So in order to do correct diagnostics here we would have to add an owning thread pointer to every event loop; then call_later() and friends can check that the owning thread is the same as the current thread. But I'm not sure that this is worth the work. (And I could see issues with the owning thread concept as well.) On Tue, Dec 16, 2014 at 4:21 PM, STINNER Victor rep...@bugs.python.org wrote: STINNER Victor added the comment: 2014-12-17 0:53 GMT+01:00 Guido van Rossum rep...@bugs.python.org: Well, the PEP clearly states that get_event_loop() should never return None and raise an exception if the context has no environment. (Where context ~~ thread.) You don't reply to my question, or I misunderstood your anwer. call_soon() is a method an event loop, there is no need to call get_event_loop(). get_event_loop() is only called in debug mode to help the developer to detect obvious bugs. I don't think that the behaviour of the debug mode must be written in the PEP, it's more implementation specific. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: I'm okay with this approach now. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Ok, here is an updated patch (version 2): - tests still ensure that the event loop is passed explicitly - call_soon() cie now fail if the policy has no event loop set for the current thread (before it only failed if the event loop set in the policy was different than the called event loop) I'm not sure that this particular change is a great idea. I kind of liked that unittests ensure that loop is passed everywhere explicitly in asyncio. I agree, but we can just monkey-patch the _assert_is_current_event_loop() to disable the check even if debug mode. For example, it can be done in test_utils.TestCase.set_event_loop(), no need to modify individual test cases. Maybe only in debug mode? Oh sorry, I forgot to mention that my patch only enhance an existing test, _assert_is_current_event_loop(), which is only done in debug mode. I'm so happy to have this debug mode, at the beginning Guido didn't want it :-D - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop Oops, the method already fails if the event loop is different than the current event loop of the policy. My patch changes the behaviour when the policy has *no* current event loop. In your patch, you should also replace 'except AsserionError' with 'except RuntimeError'. Ok, that's why we must reject any patch without unit test :-D I still think you shouldn't set the thread when the loop is created but only when run*() is active. (Using call_later() from another thread when the loop is inactive should not be an error IMO -- there's nothing against passing a selector instance to another thread as long as only one thread at a time waits for it.) Hum, I'm not fully convinced. Technically, it works to call call_soon() instead of call_soon_threadsafe() when the event loop is not running yet. But multithreading means race condition. It probably means that in most cases, calling call_soon() would work because the event loop didn't start yet, but in some cases, the event loop may already run depending on the timing. I prefer to force users to always use call_soon_threadsafe() to avoid bad suprises. -- Added file: http://bugs.python.org/file37364/asyncio_thread-2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Yury Selivanov added the comment: Hi Victor, I left you some feedback in code review. I'm kind of leaning towards your proposal that we should force users to always use safe API. But I also understand Guido's point. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Guido, Yury: What do you think? Does it sound reasonable to raise an exception if an event loop is used from the wrong thread? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: Maybe only in debug mode? Getting thread ID or current thread may be expensive. Also it is conceivable that run... is called first from one thread and then from another. Maybe. On Dec 4, 2014 4:47 PM, STINNER Victor rep...@bugs.python.org wrote: STINNER Victor added the comment: Guido, Yury: What do you think? Does it sound reasonable to raise an exception if an event loop is used from the wrong thread? -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Yury Selivanov added the comment: - modify tests to set the event loop to the newly created event loop, instead of setting it to None I'm not sure that this particular change is a great idea. I kind of liked that unittests ensure that loop is passed everywhere explicitly in asyncio. - modify get_event_loop() to always raise a RuntimeError if the thread has no event loop. Before an AssertionError was not raised if python runs with -O option +1. - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop Hm, I think I don't understand why this function doesn't to anything when there is no loop in the thread... Let's fix it. In your patch, you should also replace 'except AsserionError' with 'except RuntimeError'. Does it sound reasonable to raise an exception if an event loop is used from the wrong thread? I think we should in debug mode at least. Getting thread ID or current thread may be expensive. Victor, can you benchmark this? I'm pretty sure that Guido is right about this, but sometimes syscalls are pretty fast. I doubt that a lot of people know about debug mode, so if it has a negligible cost I'd do the check every time. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Guido van Rossum added the comment: On Thu, Dec 4, 2014 at 7:21 PM, Yury Selivanov rep...@bugs.python.org wrote: Yury Selivanov added the comment: - modify tests to set the event loop to the newly created event loop, instead of setting it to None I'm not sure that this particular change is a great idea. I kind of liked that unittests ensure that loop is passed everywhere explicitly in asyncio. Indeed, don't break that. - modify get_event_loop() to always raise a RuntimeError if the thread has no event loop. Before an AssertionError was not raised if python runs with -O option +1. +1 - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop Hm, I think I don't understand why this function doesn't to anything when there is no loop in the thread... Let's fix it. In your patch, you should also replace 'except AsserionError' with 'except RuntimeError'. Does it sound reasonable to raise an exception if an event loop is used from the wrong thread? I think we should in debug mode at least. Getting thread ID or current thread may be expensive. Victor, can you benchmark this? I'm pretty sure that Guido is right about this, but sometimes syscalls are pretty fast. I doubt that a lot of people know about debug mode, so if it has a negligible cost I'd do the check every time. I take it back, it's only 250 nsec to call threading.current_thread(), and 300 nsec to acquire and release a lock. I still think you shouldn't set the thread when the loop is created but only when run*() is active. (Using call_later() from another thread when the loop is inactive should not be an error IMO -- there's nothing against passing a selector instance to another thread as long as only one thread at a time waits for it.) -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
STINNER Victor added the comment: Here is a patch without unit: - modify get_event_loop() to always raise a RuntimeError if the thread has no event loop. Before an AssertionError was not raised if python runs with -O option - modify BaseEventLoop._assert_is_current_event_loop() to fail if the thread has an event loop - modify tests to set the event loop to the newly created event loop, instead of setting it to None -- keywords: +patch Added file: http://bugs.python.org/file37303/asyncio_thread.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue22926] asyncio: raise an exception when called from the wrong thread
Changes by STINNER Victor victor.stin...@gmail.com: -- title: asyncio: - asyncio: raise an exception when called from the wrong thread ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue22926 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
