Use an Asynchronous Event Loop in SbyJob #39
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Compared to the
select
event loop, using the event loop provided byasyncio
forsby_core
offers improvements in portability and reduces the code used for manaual event management, with minimal increase in the lines of code. Unix functionality is (almost completely- see signal handlers) unaffected, while Windows support gains feature-parity with Unix.Summary of Changes
As part of this PR,
taskloop
's semantics have changed (task
in this context meansasync def
). Instead of waiting onfd
s,taskloop
now waits onSbyTask
termination orSbyJob
timeout. The following tasks are run:maybe_spawn
will spawn a subprocess associated withSbyTask
if allSbyTasks
it depends on is finished.output
task
will collect lines ofstdout
. This happens in parallel with waiting forSbyTask
termination.shutdown_and_notify
will do cleanup and then runmaybe_spawn
on allSbyTask
s inself.notify
.timekeeper
handles global timeout ofSbyJob
.task_poller
task returns (be it termination, timeout, or completion), the event loop shuts itself down.Improvements
select
functionality.taskloop
:taskloop
will not iterate unless an event occurred (contrast to the timeout parameter ofselect
.taskloop
will make sure to attempt to start pending tasks before beginning a new iteration.Disadvantages
I had to add a workaround for Windows to ensure the Python interpreter actually exits when the event loop completes. I have no idea where in the stack to look (msys2 implementation bug? Python bug?), so I'm looking into creating an MVCE to duplicate the problem and ask for help.
That said, the code works correctly in my testing on Windows, and better than what is currently there. This is more of a complaint that a workaround is needed at all (the
tasks_retired
member ofSbyTask
exists solely for this workaround).SbyTasks
no longer spawn subprocesses; this is nowtaskloop
's responsbility. Python constructors cannot be asynchronous and the various workarounds I've found require large swaths ofSymbiYosys
to becomeasync
rather than just thetaskloop
. In my opinion, this is more trouble than it is worth for a few milliseconds less latency in getting tasks running initially. I renamedrun()
functions toinit()
to reflect the changes.Signal handlers should be async using
AbstractEventLoop.add_signal_handler()
. At present, there is no way to do this portably. This results in messages like the following when CTRL+C is pressed:Windows
Unix
This warning comparatively rare. I've not been able to cause an error on Unix.
AbstractEventLoop.add_signal_handler()
can solve this, but since signals become events in the event loop, thesys.exit(1)
line would need to be moved until after the loop finishes (i.e.task_poller
returns). Current behavior is to exit immediately inforce_shutdown
.Testing
I have tested this PR on Windows (MSYS2) and Linux (Ubuntu) using the
demo3.sby
script with an extra solver:smtbmc z3
. I also tested timeout and^C
handling.cc: @q3k and @whitequark I would also like your feedback.