Issue19566
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2013-11-13 00:19 by vstinner, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
asyncio_fastchildwatcher.patch | vstinner, 2013-11-13 15:05 | review |
Messages (14) | |||
---|---|---|---|
msg202727 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-11-13 00:19 | |
The following test of test_asyncio failed once. I didn't check if it failed more than once on this buildbot. The cleanup code is not safe, it should handle errors correctly, so following tests would not fail. http://buildbot.python.org/all/builders/x86%20Gentoo%20Non-Debug%203.x/builds/5421/steps/test/logs/stdio ====================================================================== ERROR: test_close (test.test_asyncio.test_unix_events.FastChildWatcherTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/asyncio/unix_events.py", line 662, in _do_waitpid_all callback, args = self._callbacks.pop(pid) KeyError: 7673 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_asyncio/test_unix_events.py", line 723, in setUp self.watcher = self.create_watcher(self.loop) File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_asyncio/test_unix_events.py", line 1466, in create_watcher return unix_events.FastChildWatcher(loop) File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/asyncio/unix_events.py", line 596, in __init__ super().__init__(loop) File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/asyncio/unix_events.py", line 474, in __init__ self.set_loop(loop) File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/asyncio/unix_events.py", line 498, in set_loop self._do_waitpid_all() File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/asyncio/unix_events.py", line 665, in _do_waitpid_all with self._lock: AttributeError: 'FastChildWatcher' object has no attribute '_lock' Following tests fail like that: ====================================================================== FAIL: test_create_watcher (test.test_asyncio.test_unix_events.FastChildWatcherTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_asyncio/test_unix_events.py", line 718, in setUp assert ChildWatcherTestsMixin.instance is None AssertionError |
|||
msg202729 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2013-11-13 00:23 | |
I'll ask Anthony Baire (the author of the new child watcher code) to look into this. Thanks for reporting this! |
|||
msg202756 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-11-13 15:00 | |
Also reproduced on "x86 Ubuntu Shared 3.x" buildbot. http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/9012/steps/test/logs/stdio ====================================================================== ERROR: test_close (test.test_asyncio.test_unix_events.FastChildWatcherTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/asyncio/unix_events.py", line 662, in _do_waitpid_all callback, args = self._callbacks.pop(pid) KeyError: 21303 During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_asyncio/test_unix_events.py", line 723, in setUp self.watcher = self.create_watcher(self.loop) File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_asyncio/test_unix_events.py", line 1466, in create_watcher return unix_events.FastChildWatcher(loop) File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/asyncio/unix_events.py", line 596, in __init__ super().__init__(loop) File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/asyncio/unix_events.py", line 474, in __init__ self.set_loop(loop) File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/asyncio/unix_events.py", line 498, in set_loop self._do_waitpid_all() File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/asyncio/unix_events.py", line 665, in _do_waitpid_all with self._lock: AttributeError: 'FastChildWatcher' object has no attribute '_lock' |
|||
msg202757 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-11-13 15:05 | |
Attached patch should fix this issue. BaseChildWatcher constructors calls set_loop() which calls _do_waitpid_all(). The problem is that _do_waitpid_all() is called before FastChildWatcher own attributes are set. |
|||
msg202760 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2013-11-13 16:47 | |
Hmm... That fix works, and if you're concerned about the buildbots, by all means check it in. But I think the root cause is a poor API for initializing ChildWatchers. This is currently done at the end of __init__() -- it calls self.set_loop() which is implemented by the subclass. I think the right fix is to change the protocol to separate out the constructor from the set_loop() call (which also isn't a great name, since it does so much more -- maybe it can be called link_loop()?). This is more cumbersome (esp. for the tests), but it really rubs me the wrong way that you have to to initialize the subclass before initializing the base class. |
|||
msg202767 - (view) | Author: Anthony Baire (aba) | Date: 2013-11-13 18:54 | |
I confirm the fix. It is clear that the separation between BaseChildWatcher and its subclasses is far from ideal. The first implementation was clean, but as the patch evolved interactions got complex to the point that BaseChildWatcher should not be considered as an API but rather as implementation details for the two other classes (eg. .add_child_handler() is implemented in subclasses whereas .remove_child_handler() is in the base class). At least it should be renamed as _BaseChildWatcher to make that clear. For set_loop, another possible name is 'attach_loop' (like in the doc string actually) |
|||
msg202768 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2013-11-13 18:59 | |
TBH the test structure is also rather fragile. I need to think about it more; the global state to hold the current test instance smells, as do the various class-level functions (waitpid(), WIFEXITED() etc.) that aren't methods but used as mock functions. The huge piles of mock.patch decorators should have tipped me off during the review, but I was more focused on the implementation instead of on the tests. :-( The smallest fix to prevent one breaking test from breaking all following tests is just to remove the assert. The next smallest fix is to use addCleanup() instead of tearDown() to reset ChildWatcherTestsMixin.instance. The next fix would be huge (refactor the tests completely) and I don't want to go there. Anthony, can you come up with a fix along the lines you suggested? You can submit this to the Tulip repo first (let me review it). |
|||
msg202770 - (view) | Author: Roundup Robot (python-dev) ![]() |
Date: 2013-11-13 19:09 | |
New changeset 8e0eeb4cc8fa by Guido van Rossum in branch 'default': asyncio: Temporary fix by Victor Stinner for issue 19566. http://hg.python.org/cpython/rev/8e0eeb4cc8fa |
|||
msg202771 - (view) | Author: Guido van Rossum (gvanrossum) * ![]() |
Date: 2013-11-13 19:09 | |
I pushed Victor's temporary patch so the buildbots can have peace. |
|||
msg202772 - (view) | Author: Anthony Baire (aba) | Date: 2013-11-13 19:29 | |
I put a cleaner patch here: https://codereview.appspot.com/26220043/ |
|||
msg202776 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-11-13 20:52 | |
> Hmm... That fix works, and if you're concerned about the buildbots, by all means check it in. > (...) > I pushed Victor's temporary patch so the buildbots can have peace. Yes, I'm concerned by buildbots, I would like to check if my last changes did not introduce any regression. This week, there is a lot of noise: many false positive related to networks and various other bugs. |
|||
msg202777 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-11-13 20:54 | |
(So thanks for having applied my fix.) |
|||
msg202801 - (view) | Author: Roundup Robot (python-dev) ![]() |
Date: 2013-11-13 23:50 | |
New changeset eb42adc53923 by Guido van Rossum in branch 'default': asyncio: Fix from Anthony Baire for CPython issue 19566 (replaces earlier fix). http://hg.python.org/cpython/rev/eb42adc53923 |
|||
msg206001 - (view) | Author: STINNER Victor (vstinner) * ![]() |
Date: 2013-12-13 00:36 | |
I think the issue has been fixed, thanks. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:53 | admin | set | github: 63765 |
2013-12-13 00:36:41 | vstinner | set | status: open -> closed resolution: fixed messages: + msg206001 |
2013-11-13 23:50:15 | python-dev | set | messages: + msg202801 |
2013-11-13 20:54:00 | vstinner | set | messages: + msg202777 |
2013-11-13 20:52:14 | vstinner | set | messages: + msg202776 |
2013-11-13 19:29:16 | aba | set | messages: + msg202772 |
2013-11-13 19:09:38 | gvanrossum | set | messages: + msg202771 |
2013-11-13 19:09:10 | python-dev | set | nosy:
+ python-dev messages: + msg202770 |
2013-11-13 18:59:20 | gvanrossum | set | messages: + msg202768 |
2013-11-13 18:54:10 | aba | set | nosy:
+ aba messages: + msg202767 |
2013-11-13 16:47:00 | gvanrossum | set | messages: + msg202760 |
2013-11-13 15:05:05 | vstinner | set | files:
+ asyncio_fastchildwatcher.patch keywords: + patch messages: + msg202757 |
2013-11-13 15:00:16 | vstinner | set | messages: + msg202756 |
2013-11-13 00:23:51 | gvanrossum | set | messages: + msg202729 |
2013-11-13 00:19:37 | vstinner | create |