On Aug. 1, 2014, 9:14 p.m., Miloslav Trmac wrote:
> src/rolekit/async.py, line 193
>
<
http://reviewboard-fedoraserver.rhcloud.com/r/62/diff/1/?file=253#file253...
>
> Actually, this might be a major issue with using threads.
>
> The way Future objects work, set_result()/set_exception() directly calls all the
registered callbacks, which include async_step above, and that calls the next step within
the coroutine.
>
> So, when the worker thread finishes working with the subprocess, it calls
future.set_result() and that ends up causing _the next part of the coroutine calling
async_subprocess_ to run within that thread as well, completely unsynchronized with any
other async code in roled and quite possibly accessing the same data (e.g. the instance
objects).
>
> Using the GLib design of
thread-safety-by-no-locking-and-executing-everything-on-the-same-thread, we would need to
ensure that the next part of the coroutine runs on the main thread, i.e. creating a
GSource that poll()s for something set by the thread worker; using (logically) a condition
variable, or (usual implementation?) a helper pipe and unix_fd_source_new.
>
> This is starting to get complicated…
Miloslav Trmac wrote:
… because I am probably overcomplicating it, it’s too late. The worker thread can
call the Python equivalent of g_idle_add() to cause code to be run on the main process
(and the code run on the main process would be what calls set_result or a set_exception on
the future returned from async_subprocess; and that’s a different future from the one
returned by .submit(), we don’t really need that one AFAICT).
I'll be honest here, I'm not following at all.
Please re-review with my updated patch and tell me if it changes anything.
- Stephen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/62/#review208
-----------------------------------------------------------
On Aug. 1, 2014, 7:02 p.m., Stephen Gallagher wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-fedoraserver.rhcloud.com/r/62/
-----------------------------------------------------------
(Updated Aug. 1, 2014, 7:02 p.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas
Woerner.
Repository: rolekit
Description
-------
This will spawn a thread to monitor the subprocess, capture both
stdout and stderr and throw an exception if the process terminates
with a non-zero error code.
Diffs
-----
src/rolekit/async.py 1e2b82eab5a4e6862594672f738c1935a83c2be5
Diff:
http://reviewboard-fedoraserver.rhcloud.com/r/62/diff/
Testing
-------
Manual testing was done by having it call a script that slept for 30s and verifying that
other requests could be successfully made to rolekit while it was waiting.
This code probably needs a unit test, but I haven't written it yet.
Thanks,
Stephen Gallagher