Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24456002:
[Android] Fork a detached child process from adb_logcat_monitor.py. (Closed)
|
Created:
7 years, 3 months ago by Siva Chandra Modified:
7 years, 2 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Fork a detached child process from adb_logcat_monitor.py.
This way, we can invoke adb_logcat_monitor.py directly from
recipes. We cannot spawn a process which runs
adb_logcat_monitor.py (like we have been via
buildbot/bb_device_steps.py) from recipes as we cannot import
subprocess et al. from recipes.
BUG=286509
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226151
Patch Set 1 #
Total comments: 12
Patch Set 2 : Exit parent process with os._exit #
Total comments: 9
Patch Set 3 : Removed the spurious return #
Messages
Total messages: 34 (0 generated)
https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (left): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:288: bb_utils.SpawnCmd([ Why can't we spawn this command via SpawnCmd from recipes as we did before recipes rather than running a script that forks a detached process? I'm sure there is a reason I don't know. Please update the description of the CL with that information too.
https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... File build/android/buildbot/bb_device_steps.py (left): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... build/android/buildbot/bb_device_steps.py:288: bb_utils.SpawnCmd([ On 2013/09/24 23:36:16, navabi wrote: > Why can't we spawn this command via SpawnCmd from recipes as we did before > recipes rather than running a script that forks a detached process? > > I'm sure there is a reason I don't know. Please update the description of the CL > with that information too. Updated the CL description.
On 2013/09/24 23:47:05, Siva Chandra wrote: > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > File build/android/buildbot/bb_device_steps.py (left): > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > build/android/buildbot/bb_device_steps.py:288: bb_utils.SpawnCmd([ > On 2013/09/24 23:36:16, navabi wrote: > > Why can't we spawn this command via SpawnCmd from recipes as we did before > > recipes rather than running a script that forks a detached process? > > > > I'm sure there is a reason I don't know. Please update the description of the > CL > > with that information too. > > Updated the CL description. (on that note, there are plans to allow for parallel step execution, which would let you do this as a daemon script)
On 2013/09/25 00:52:14, iannucci wrote: > On 2013/09/24 23:47:05, Siva Chandra wrote: > > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > > File build/android/buildbot/bb_device_steps.py (left): > > > > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > > build/android/buildbot/bb_device_steps.py:288: bb_utils.SpawnCmd([ > > On 2013/09/24 23:36:16, navabi wrote: > > > Why can't we spawn this command via SpawnCmd from recipes as we did before > > > recipes rather than running a script that forks a detached process? > > > > > > I'm sure there is a reason I don't know. Please update the description of > the > > CL > > > with that information too. > > > > Updated the CL description. > > (on that note, there are plans to allow for parallel step execution, which would > let you do this as a daemon script) (https://code.google.com/p/chromium/issues/detail?id=297861 for those who are interested)
On 2013/09/25 00:52:43, iannucci wrote: > On 2013/09/25 00:52:14, iannucci wrote: > > On 2013/09/24 23:47:05, Siva Chandra wrote: > > > > > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > > > File build/android/buildbot/bb_device_steps.py (left): > > > > > > > > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/buildbot... > > > build/android/buildbot/bb_device_steps.py:288: bb_utils.SpawnCmd([ > > > On 2013/09/24 23:36:16, navabi wrote: > > > > Why can't we spawn this command via SpawnCmd from recipes as we did before > > > > recipes rather than running a script that forks a detached process? > > > > > > > > I'm sure there is a reason I don't know. Please update the description of > > the > > > CL > > > > with that information too. > > > > > > Updated the CL description. > > > > (on that note, there are plans to allow for parallel step execution, which > would > > let you do this as a daemon script) > > (https://code.google.com/p/chromium/issues/detail?id=297861 for those who are > interested) That makes sense. Thanks Siva and Robbie for the explanation + information. lgtm
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/24456002/diff/1/build/android/adb_logcat_moni... File build/android/adb_logcat_monitor.py (right): https://codereview.chromium.org/24456002/diff/1/build/android/adb_logcat_moni... build/android/adb_logcat_monitor.py:105: os.setsid() How does this process get cleaned up?
https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:105: os.setsid() On 2013/09/26 18:15:57, iannucci wrote: > How does this process get cleaned up? There is another step which is always executed. That step cleans this up.
Changing this to a fork seems dangerous and I'm not it improves the API. Since I'm on another timezone I won't block this change -- but please wait for iannucci to approve too. https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:102: return 0 yikes. I believe this needs to be os._exit(0) https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:105: os.setsid() On 2013/09/26 18:15:57, iannucci wrote: > How does this process get cleaned up? Another process reads PID from a file and kills it. An alternative here would be to revamp the way the logger works -- which might less error prone than using a raw fork (which again, is really dangerous).
On 2013/09/26 18:31:08, Isaac wrote: > Changing this to a fork seems dangerous and I'm not it improves the API. Since > I'm on another timezone I won't block this change -- but please wait for > iannucci to approve too. > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... > File build/android/adb_logcat_monitor.py (right): > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... > build/android/adb_logcat_monitor.py:102: return 0 > yikes. I believe this needs to be os._exit(0) > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... > build/android/adb_logcat_monitor.py:105: os.setsid() > On 2013/09/26 18:15:57, iannucci wrote: > > How does this process get cleaned up? > > Another process reads PID from a file and kills it. An alternative here would > be to revamp the way the logger works -- which might less error prone than using > a raw fork (which again, is really dangerous). Not sure how it's 'really dangerous'? I would say 'slightly trickier', unless you know something I don't?
Let's make sure this process is properly cleaned up. https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:105: os.setsid() On 2013/09/26 18:31:08, Isaac wrote: > On 2013/09/26 18:15:57, iannucci wrote: > > How does this process get cleaned up? > > Another process reads PID from a file and kills it. An alternative here would > be to revamp the way the logger works -- which might less error prone than using > a raw fork (which again, is really dangerous). Where does that happen again? I don't see the PID being written to a file. Can you point me to the step that is executed and cleans this up? Is it in a finally block?
https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:102: return 0 On 2013/09/26 18:31:08, Isaac wrote: > yikes. I believe this needs to be os._exit(0) I think in principle this should be the other way: child process should exit os._exit(...). However, in this case, we are always doing this right at the beginning and always guaranteed that the parent process dies before the child without creating any shared resources. Hence, exit from parent and child seem safe enough. This is based on my understanding. You can explain more and help me understand better if I got it wrong.
https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:105: os.setsid() On 2013/09/26 19:56:00, navabi wrote: > On 2013/09/26 18:31:08, Isaac wrote: > > On 2013/09/26 18:15:57, iannucci wrote: > > > How does this process get cleaned up? > > > > Another process reads PID from a file and kills it. An alternative here would > > be to revamp the way the logger works -- which might less error prone than > using > > a raw fork (which again, is really dangerous). > > Where does that happen again? I don't see the PID being written to a file. > Can you point me to the step that is executed and cleans this up? Is it in a > finally block? > See below. https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:133: with open(pid_file_path, 'w') as f: The PID gets written to a file here. https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:147: logging.info('Received SIGTERM, shutting down') adb_logcat_printer.py:133 sends SIGTERM which is caught here. https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... build/android/adb_logcat_monitor.py:157: os.remove(pid_file_path) The PID file is removed here.
On 2013/09/26 20:24:12, Siva Chandra wrote: > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... > File build/android/adb_logcat_monitor.py (right): > > https://chromiumcodereview.appspot.com/24456002/diff/1/build/android/adb_logc... > build/android/adb_logcat_monitor.py:102: return 0 > On 2013/09/26 18:31:08, Isaac wrote: > > yikes. I believe this needs to be os._exit(0) > > I think in principle this should be the other way: child process should exit > os._exit(...). However, in this case, we are always doing this right at the > beginning and always guaranteed that the parent process dies before the child > without creating any shared resources. Hence, exit from parent and child seem > safe enough. This is based on my understanding. You can explain more and help me > understand better if I got it wrong. I'm not super familiar, but here's a quote from a unix guide: http://www.unixguide.net/unix/programming/1.1.3.shtml """ There are some unusual cases, like daemons, where the parent should call _exit() rather than the child; the basic rule, applicable in the overwhelming majority of cases, is that exit() should be called only once for each entry into main. """
Here's a python daemon recipe. We probably don't need its full behavior but note that it uses os._exit on parents. http://code.activestate.com/recipes/278731-creating-a-daemon-the-python-way/
On 2013/09/26 21:03:45, Isaac wrote: > Here's a python daemon recipe. We probably don't need its full behavior but > note that it uses os._exit on parents. OK. I do not understand well enough here. So, let me stick to the principles. I have added an os._exit to the parent in the latest patch set.
https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:102: return os._exit(os.EX_OK) I don't think this return will ever actually happen since os._exit will detonate the process immediately. https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:111: shutil.rmtree(base_dir, ignore_errors=True) Shouldn't we only be doing this sort of thing AFTER we've already killed off the existing process (if any)? https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... build/android/buildbot/bb_device_steps.py:293: RunCmd(['sleep', '5']) Ew. Really? We can't monitor the pidfile?
Welcome to the wild west? This was one of my first python scripts on chrome team; it's not clean but it works good [sic]. https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:111: shutil.rmtree(base_dir, ignore_errors=True) On 2013/09/26 21:30:57, iannucci wrote: > Shouldn't we only be doing this sort of thing AFTER we've already killed off the > existing process (if any)? That's why we ignore errors! :-) https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... build/android/buildbot/bb_device_steps.py:293: RunCmd(['sleep', '5']) On 2013/09/26 21:30:57, iannucci wrote: > Ew. Really? We can't monitor the pidfile? Yeah I think this could be done in the logcat printer instead if it notices the pid file was created within 5 seconds.
https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:102: return os._exit(os.EX_OK) On 2013/09/26 21:30:57, iannucci wrote: > I don't think this return will ever actually happen since os._exit will detonate > the process immediately. Didn't understand. The child process is already forked at this point is it not?
https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:102: return os._exit(os.EX_OK) On 2013/09/26 22:47:57, Siva Chandra wrote: > On 2013/09/26 21:30:57, iannucci wrote: > > I don't think this return will ever actually happen since os._exit will > detonate > > the process immediately. > > Didn't understand. The child process is already forked at this point is it not? s/return //
https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... File build/android/adb_logcat_monitor.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... build/android/adb_logcat_monitor.py:102: return os._exit(os.EX_OK) On 2013/09/26 23:57:29, iannucci wrote: > On 2013/09/26 22:47:57, Siva Chandra wrote: > > On 2013/09/26 21:30:57, iannucci wrote: > > > I don't think this return will ever actually happen since os._exit will > > detonate > > > the process immediately. > > > > Didn't understand. The child process is already forked at this point is it > not? > > s/return // Still did not undertstand :(
On 2013/09/27 00:54:59, Siva Chandra wrote: > https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... > File build/android/adb_logcat_monitor.py (right): > > https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/adb_... > build/android/adb_logcat_monitor.py:102: return os._exit(os.EX_OK) > On 2013/09/26 23:57:29, iannucci wrote: > > On 2013/09/26 22:47:57, Siva Chandra wrote: > > > On 2013/09/26 21:30:57, iannucci wrote: > > > > I don't think this return will ever actually happen since os._exit will > > > detonate > > > > the process immediately. > > > > > > Didn't understand. The child process is already forked at this point is it > > not? > > > > s/return // > > Still did not undertstand :( the word 'return' on that line is not needed, since os._exit will not return (the process will terminate instead).
> the word 'return' on that line is not needed, since os._exit will not return > (the process will terminate instead). stupid me! got it!
https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... File build/android/buildbot/bb_device_steps.py (right): https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... build/android/buildbot/bb_device_steps.py:293: RunCmd(['sleep', '5']) On 2013/09/26 21:45:30, Isaac wrote: > On 2013/09/26 21:30:57, iannucci wrote: > > Ew. Really? We can't monitor the pidfile? > > Yeah I think this could be done in the logcat printer instead if it notices the > pid file was created within 5 seconds. Based on the comment, this is not waiting for the pidfile to be created within 5 seconds (which makes sense as 5 seconds would be way too long to wait for the file to be created). It is waiting to make sure the initial calls to "adb logcat -c" has completed on all attached devices. I'm not sure how monitoring the pidfile would help with that.
On 2013/09/27 20:38:54, navabi wrote: > https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... > File build/android/buildbot/bb_device_steps.py (right): > > https://chromiumcodereview.appspot.com/24456002/diff/21001/build/android/buil... > build/android/buildbot/bb_device_steps.py:293: RunCmd(['sleep', '5']) > On 2013/09/26 21:45:30, Isaac wrote: > > On 2013/09/26 21:30:57, iannucci wrote: > > > Ew. Really? We can't monitor the pidfile? > > > > Yeah I think this could be done in the logcat printer instead if it notices > the > > pid file was created within 5 seconds. > > Based on the comment, this is not waiting for the pidfile to be created within 5 > seconds (which makes sense as 5 seconds would be way too long to wait for the > file to be created). It is waiting to make sure the initial calls to "adb logcat > -c" has completed on all attached devices. > > I'm not sure how monitoring the pidfile would help with that. We can fix the sleep later.
The latest patch set removes the spurious return that iannucci@ pointed out.
On 2013/09/27 23:34:57, Siva Chandra wrote: > The latest patch set removes the spurious return that iannucci@ pointed out. lgtm
On 2013/09/28 00:31:39, navabi wrote: > On 2013/09/27 23:34:57, Siva Chandra wrote: > > The latest patch set removes the spurious return that iannucci@ pointed out. > > lgtm lgtm (from my chromium.org account)
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm as long as we're confident that this process will get reaped correctly.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sivachandra@chromium.org/24456002/37001
Message was sent while issue was closed.
Change committed as 226151 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
