Issue 67400043: Ensure call_soon() invoked on current loop when get_debug() enabled
|
Created:
10 years, 2 months ago by davidfstr Modified:
10 years, 1 month ago CC:
python-tulip_googlegroups.com Visibility:
Public. |
DescriptionEnsure call_soon() invoked on current loop when get_debug() enabled
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add check to call_at() and call_later(). Add unit tests. #Patch Set 3 : Integrate inline comments from patch set 1. #
Total comments: 11
Patch Set 4 : Private keyword parameter eliminated. Integrate other feedback. #
Total comments: 2
Patch Set 5 : Preoptimize to avoid get_debug() and _assert_is_current_event_loop() calls on main path. #
Total comments: 6
Patch Set 6 : Integrate more feedback. #Patch Set 7 : Remove wrong_thread=False tests. Inline and split remaining 2 tests. #MessagesTotal messages: 18
The assertion introduced by this change has already been helpful in identifying incorrect thread usage in a prototype @thread_affinity decorator I have been writing. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode281 asyncio/base_events.py:281: "call_soon() invoked on event loop other than current one") I would prefer to use a more specific exception such as an equivalent to Java's IllegalStateException, but I am not aware of any such exception for Python.
Sign in to reply to this message.
I opened http://code.google.com/p/tulip/issues/detail?id=154 to track this. General comment: shouldn't call_at() and call_later() have the same check? https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode281 asyncio/base_events.py:281: "call_soon() invoked on event loop other than current one") On 2014/02/23 06:28:21, davidfstr wrote: > I would prefer to use a more specific exception such as an equivalent to Java's > IllegalStateException, but I am not aware of any such exception for Python. I'd use RuntimeError.
Sign in to reply to this message.
We'll also need unit-tests. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode266 asyncio/base_events.py:266: def call_soon(self, callback, *args, _check_thread=True): '__check_thread' to make it harder to use this API from the outside of asyncio? https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode278 asyncio/base_events.py:278: if _check_thread and self.get_debug() and \ You should use '_debug' here. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode279 asyncio/base_events.py:279: events.get_event_loop() != self: I think it's better to memorize the 'thread.ident' in loop's constructor and check it here. Otherwise, if a user has a custom event loop policy, you theoretically can create a new event loop in another thread by calling 'get_event_loop'. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode281 asyncio/base_events.py:281: "call_soon() invoked on event loop other than current one") On 2014/02/23 23:38:25, GvR wrote: > On 2014/02/23 06:28:21, davidfstr wrote: > > I would prefer to use a more specific exception such as an equivalent to > Java's > > IllegalStateException, but I am not aware of any such exception for Python. > > I'd use RuntimeError. +1
Sign in to reply to this message.
Responded to inline comments on patch set 1. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode266 asyncio/base_events.py:266: def call_soon(self, callback, *args, _check_thread=True): On 2014/02/24 02:05:20, yselivanov wrote: > '__check_thread' to make it harder to use this API from the outside > of asyncio? Making it __check_thread creates strange errors: Traceback (most recent call last): File "/Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/concurrent/futures/_base.py", line 297, in _invoke_callbacks callback(self) File "/Users/davidf/Projects/tulip/asyncio/futures.py", line 370, in <lambda> new_future._copy_state, fut)) File "/Users/davidf/Projects/tulip/asyncio/base_events.py", line 297, in call_soon_threadsafe handle = self.call_soon(callback, *args, __check_loop=False) TypeError: call_soon() got an unexpected keyword argument '__check_loop' Despite the error message, call_soon() definitely has a keyword argument '__check_loop'. I suspect some form of name mangling but I don't know how such mangling with work with keyword arguments. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode278 asyncio/base_events.py:278: if _check_thread and self.get_debug() and \ On 2014/02/24 02:05:20, yselivanov wrote: > You should use '_debug' here. Why prefer a private field over a public method? https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode279 asyncio/base_events.py:279: events.get_event_loop() != self: On 2014/02/24 02:05:20, yselivanov wrote: > I think it's better to memorize the 'thread.ident' in loop's constructor > and check it here. Otherwise, if a user has a custom event loop policy, > you theoretically can create a new event loop in another thread > by calling 'get_event_loop'. Couldn't an event loop be run on different threads at different times? For example thread alpha calls loop.run_until_complete() and later thread beta calls loop.run_forever(). In that case it would not be safe to memorize the initial thread as a proxy for identifying the current event loop. It's a shame that get_event_loop() cannot be used to safely detect the absence of a current loop. https://codereview.appspot.com/67400043/diff/1/asyncio/base_events.py#newcode281 asyncio/base_events.py:281: "call_soon() invoked on event loop other than current one") On 2014/02/24 02:05:20, yselivanov wrote: > On 2014/02/23 23:38:25, GvR wrote: > > On 2014/02/23 06:28:21, davidfstr wrote: > > > I would prefer to use a more specific exception such as an equivalent to > > Java's > > > IllegalStateException, but I am not aware of any such exception for Python. > > > > I'd use RuntimeError. > > +1 Done. A more specific subclass may be used in the future.
Sign in to reply to this message.
The strange error message about __check_thread is because of the private name mangling. Try using a single leading _.
Sign in to reply to this message.
https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:240: def call_later(self, delay, callback, *args, _check_loop=True): I don't understand why you added a parameter here. I don't see why you would call call_later(..., _check_loop=False) https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:257: self._assert_is_current_event_loop() Why not relying on call_at() check? It would avoid the need of adding a private parameter. https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:261: def call_at(self, when, callback, *args, _check_loop=True): IMO you should remove the parameter. https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:265: if _check_loop: You can move the check on the debug flag here, for better performances: if self._debug: self._check_event_loop() https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:271: def call_soon(self, callback, *args, _check_loop=True): I don't like this private parameter, I would prefer a new submethod: def _call_soon(self, callback, args, check_event_loop): if self._debug and check_event_loop: self._check_event_loop() ... And call it from call_soon_threadsafe() and call_soon(). https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:289: def _assert_is_current_event_loop(self): I don't understand the term "assert" in the function name you proposed, it's not an assertion (but you used AssertionError in a previous version of your patch). I propose to rename the method to "_check_event_loop". You can move it up to define it before you use it, so before call_later(). https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:290: if self.get_debug() and events.get_event_loop() != self: You may write "events.get_event_loop() is not self". (Drop self.get_debug() test, move it to the caller as self._debug.) Note: Calling events.get_event_loop() in the main thread creates an event loop if the current event loop is running in a dedicated thread and the main thread didn't have an event loop. Since the check is only done in debug mode, it's fine. https://codereview.appspot.com/67400043/diff/40001/asyncio/base_events.py#new... asyncio/base_events.py:292: "non-threadsafe operation invoked on event loop other than " + "on *an* event loop"? https://codereview.appspot.com/67400043/diff/40001/asyncio/test_utils.py File asyncio/test_utils.py (right): https://codereview.appspot.com/67400043/diff/40001/asyncio/test_utils.py#newc... asyncio/test_utils.py:353: def call_at(self, when, callback, *args, **kwargs): This is exactly why I'm against using private keyword parameter, it makes the API uglier. https://codereview.appspot.com/67400043/diff/40001/tests/test_base_events.py File tests/test_base_events.py (right): https://codereview.appspot.com/67400043/diff/40001/tests/test_base_events.py#... tests/test_base_events.py:159: (wrong_thread and debug) else EmptyContextManager() Other may disagree, but why not only testing the case which must raise RuntimeError? It would avoid EmptyContextManager. https://codereview.appspot.com/67400043/diff/40001/tests/test_base_events.py#... tests/test_base_events.py:941: (Space) change unrelated to the issue, it should be reverted.
Sign in to reply to this message.
New diff uploaded: Private keyword parameter eliminated. Integrate other feedback. # (edit)
Sign in to reply to this message.
https://codereview.appspot.com/67400043/diff/60001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/60001/asyncio/base_events.py#new... asyncio/base_events.py:262: self._assert_is_current_event_loop() IMO you should move the check to the debug flag here: if self._debug: self._assert_is_current_event_loop() It would reduce the overhead in release mode (function calls in Python are "expensive"). https://codereview.appspot.com/67400043/diff/60001/asyncio/base_events.py#new... asyncio/base_events.py:282: if check_loop: Ditto: if check_loop and self._debug: ...
Sign in to reply to this message.
New diff uploaded: Preoptimize to avoid get_debug() and _assert_is_current_event_loop() calls on main path.
Sign in to reply to this message.
The patch looks good to me, except of 2 minor nits that can be fixed when the patch is applied. If Guido and/or Yury is ok, I will apply the patch. https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py#new... asyncio/base_events.py:278: return self._call_soon(callback, *args, check_loop=True) No need to "pack"/"unpack" args using *args here. Just pass the tuple of args. https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py#new... asyncio/base_events.py:293: "non-threadsafe operation invoked on an event loop other " + no need for "+" operator here.
Sign in to reply to this message.
https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py#new... asyncio/base_events.py:278: return self._call_soon(callback, *args, check_loop=True) On 2014/03/19 10:07:18, haypo_gmail wrote: > No need to "pack"/"unpack" args using *args here. Just pass the tuple of args. +1. https://codereview.appspot.com/67400043/diff/80001/asyncio/base_events.py#new... asyncio/base_events.py:290: def _assert_is_current_event_loop(self): Please add a docstring. https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py File tests/test_base_events.py (right): https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py#... tests/test_base_events.py:139: def test_call_functions_detect_wrong_thread(self): I'd break this test into 2 separate tests. There is no point in DRY here.
Sign in to reply to this message.
New diff posted: Integrated more feedback. I am learning more about making performance optimizations in Python than I expected. :-) https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py File tests/test_base_events.py (right): https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py#... tests/test_base_events.py:139: def test_call_functions_detect_wrong_thread(self): Is there a downside to DRY here? This seems the most straightforward way to test the combinatorics of this method. If this were to be split, along which dimension and why? On 2014/03/19 14:52:48, yselivanov wrote: > I'd break this test into 2 separate tests. There is no point in DRY here.
Sign in to reply to this message.
On 2014/03/20 04:07:44, davidfstr wrote: > New diff posted: Integrated more feedback. > > I am learning more about making performance optimizations in Python than I > expected. :-) > > https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py > File tests/test_base_events.py (right): > > https://codereview.appspot.com/67400043/diff/80001/tests/test_base_events.py#... > tests/test_base_events.py:139: def > test_call_functions_detect_wrong_thread(self): > Is there a downside to DRY here? This seems the most straightforward way to test > the combinatorics of this method. > > If this were to be split, along which dimension and why? > > On 2014/03/19 14:52:48, yselivanov wrote: > > I'd break this test into 2 separate tests. There is no point in DRY here. I'd simply have two unit tests. One for when debug is ON, and another one when it's OFF. Downsides of the current approach: 1. EmptyContextManager 2. Complicated logic with for-loops etc around just three lines of the actual test 3. if you decide later to extend the test for debug-on, you'll need to put an 'if' statement in your 'for' loop. 4. What's the point of having 'wrong_thread -> False' test? All asyncio test suite is about testing in this mode. All in all, I find the test code unnecessary complex. If you break it down into two tests, you'll have two extremely simple test functions, one with ~3 lines of code, and other one with ~6.
Sign in to reply to this message.
New diff uploaded: Remove wrong_thread=False tests. Inline and split remaining 2 tests.
Sign in to reply to this message.
On 2014/03/21 02:43:27, davidfstr wrote: > New diff uploaded: Remove wrong_thread=False tests. Inline and split remaining 2 > tests. LGTM
Sign in to reply to this message.
On 2014/03/21 03:40:42, yselivanov wrote: > On 2014/03/21 02:43:27, davidfstr wrote: > > New diff uploaded: Remove wrong_thread=False tests. Inline and split remaining > 2 > > tests. > > LGTM I applied the patch to Tulip (3d4d0da510e0) and CPython 3.5 (30977e66a882). IMO it would be nice to have this tool in CPython 3.4, what do you think?
Sign in to reply to this message.
On 2014/03/21 09:02:31, haypo_gmail wrote: > IMO it would be nice to have this tool in CPython 3.4, what do you think? I think that makes sense and I don't see any reason why you can't, so go ahead. (Are there other things that could be merged into the 3.4 branch?)
Sign in to reply to this message.
On 2014/03/24 22:09:22, GvR wrote: > On 2014/03/21 09:02:31, haypo_gmail wrote: > > IMO it would be nice to have this tool in CPython 3.4, what do you think? > > I think that makes sense and I don't see any reason why you can't, so go ahead. > (Are there other things that could be merged into the 3.4 branch?) Ok, patch applied to Python 3.4 (458181a8b48a). I don't know other asyncio changes which are specific to Python 3.5.
Sign in to reply to this message.
|
