----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ ----------------------------------------------------------- (Updated Srp. 1, 2014, 9:02 odp.) 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

On Srp. 1, 2014, 11:14 odp., 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…

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ ----------------------------------------------------------- (Updated Srp. 1, 2014, 9:02 odp.) 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

On Srp. 1, 2014, 11:14 odp., 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). Stephen Gallagher wrote: 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.

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ ----------------------------------------------------------- (Updated Srp. 4, 2014, 1:27 odp.) 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

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ ----------------------------------------------------------- (Updated Srp. 4, 2014, 1:27 odp.) 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

----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ ----------------------------------------------------------- (Updated Srp. 4, 2014, 1:27 odp.) 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