----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/ -----------------------------------------------------------
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/#review207 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment114
My OCD would like these to be alphabetically-sorted… (and I know I broke that first, sorry).
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment115
Does this actually work?
From my reading of Future (but not testing this!), the callback is run somewhere within the worker thread (inside future.set_result() -> future._invoke_callbacks()), and an exception is logged and ignored.
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment116
Same here.
AFAICT this would be simplest with a helper function that does p.communicate + the handling in finish_subprocess, and submitting that helper function to the executor; then the helper can simply return or raise exceptions.
And async_subprocess could return the future returned by executor.submit directly to the caller.
- Miloslav Trmac
On Srp. 1, 2014, 9:02 odp., Stephen Gallagher wrote:
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
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/#review208 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment117
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
On Srp. 1, 2014, 9:02 odp., Stephen Gallagher wrote:
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#file253line193
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…
… 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).
- Miloslav
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/#review208 -----------------------------------------------------------
On Srp. 1, 2014, 9:02 odp., Stephen Gallagher wrote:
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 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#file253line193
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
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#file253line193
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.
No, the issue persists.
- Miloslav
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/#review208 -----------------------------------------------------------
On Srp. 4, 2014, 1:27 odp., Stephen Gallagher wrote:
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 Aug. 4, 2014, 11:27 a.m.)
Review request for RoleKit Mailing List, Miloslav Trmac, Stephen Gallagher, and Thomas Woerner.
Changes -------
Reorder the __all__ variable. Fix reporting return codes from the subprocess (generating an exception does not work)
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 (updated) -----
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/#review214 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment126
If the first raise didn’t work, this one doesn’t either. AFAICT all of the finish_subprocess callback should be dropped. If you want the logging, do it in communicate_helper.
- Miloslav Trmac
On Srp. 4, 2014, 1:27 odp., Stephen Gallagher wrote:
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/#review216 -----------------------------------------------------------
src/rolekit/async.py http://reviewboard-fedoraserver.rhcloud.com/r/62/#comment128
You're right, the 'raise' is useless here and it could all be logged in the communicate_helper()
- Stephen Gallagher
On Aug. 4, 2014, 11:27 a.m., Stephen Gallagher wrote:
This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/
(Updated Aug. 4, 2014, 11:27 a.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
----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard-fedoraserver.rhcloud.com/r/62/#review220 -----------------------------------------------------------
See #65 , which does everything on the main thread (at the cost of some ugliness around both GLib and subprocess wanting to use waitpid()).
- Miloslav Trmac
On Srp. 4, 2014, 1:27 odp., Stephen Gallagher wrote:
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 Aug. 19, 2014, 11:49 a.m.)
Status ------
This change has been discarded.
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
rolekit-commits@lists.fedorahosted.org