Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix event loop closed bug #198

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Fix event loop closed bug #198

merged 1 commit into from
Mar 12, 2019

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Mar 11, 2019

Fixes #185. Apparently, atexit registered functions get run before the python's gc runs objects destructors and thus asyncio loops were being closed before ExecuteProcess related awaitables were terminated.

This PR also fixes an issue introduced by #167 on Python 3.5 (in Xenial).

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic requested review from wjwwood and ivanpauno March 11, 2019 20:11
@hidmic hidmic added in progress Actively being worked on (Kanban column) in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Mar 11, 2019
@dirk-thomas
Copy link
Member

This PR also fixes an issue introduced by #167 on Python 3.5 (in Xenial).

This looks like a duplicate of #196 and shouldn't be necessary since Xenial / Python 3.5 isn't a supported platform on master anymore.

@hidmic
Copy link
Contributor Author

hidmic commented Mar 11, 2019

Fair enough, I'll drop that commit.

@hidmic hidmic force-pushed the hidmic/fix-closed-event-loop branch from fe04fe2 to 670872b Compare March 11, 2019 20:27
@wjwwood
Copy link
Member

wjwwood commented Mar 11, 2019

This patch is definitely more defensive, but I'd be interested to see if the atexit handler is even needed anymore, see:

# This atexit handler ensures all the loops are closed at exit.
# This is only really required pre-3.6, see:
# https://github.com/ros2/launch/issues/84
# https://bugs.python.org/issue23548

@hidmic
Copy link
Contributor Author

hidmic commented Mar 12, 2019

This patch is definitely more defensive, but I'd be interested to see if the atexit handler is even needed anymore

It is on Python 3.5, that bug is still there. One could argue, as Dirk did, that we're not targeting Ubuntu Xenial anymore and (transitively) neither Python 3.5, and thus we should probably drop that handler. I didn't do it to keep this PR small and the discussion brief :)

@hidmic hidmic merged commit 1347bed into master Mar 12, 2019
@hidmic hidmic deleted the hidmic/fix-closed-event-loop branch March 12, 2019 12:49
@hidmic hidmic removed the in review Waiting for review (Kanban column) label Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asyncio traceback when closing
3 participants