-
Notifications
You must be signed in to change notification settings - Fork 782
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
runner fixes and enhancements #2053
Conversation
loop = asyncio.get_running_loop() | ||
loop.add_signal_handler( | ||
signal.SIGINT, | ||
lambda: self._handle_sigint(signum=signal.SIGINT, frame=None), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be better with asyncio.create_task
and a wrapped async signal handler.
By using a synchronous one with time.sleep
and subprocess.check_call
, instead of awaiting on asyncio.sleep
and asyncio.create_subprocess_exec
, you're blocking other coroutines from running in the meantime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @filipcacky, would you like to contribute this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, either you can push on top of my branch / fork..
OR we can merge this and you can raise another.. what say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, sorry, I'm from EU so it's a bit late for me 🙂 I've implemented this in async in my MR. You can copy it over from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, you can contribute tomorrow as discussed in DM 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't have permissions to push/create PR to your remote, so I added my changes here filipcacky@5dd4aab. Feel free to cherry-pick.
This reverts commit cbfaea9.
This PR is inspired by a few ideas from
poll()
not available for theProcess
object created async)It also comes with: