Adding a server side, non-blocking subprocess mechanism.

Adding a server side, non-blocking subprocess mechanism.

BUG=453679
Committed: https://crrev.com/759bbd2f8ecc15d79a67a7e368819ab422a10cb3
Cr-Commit-Position: refs/heads/master@{#316594}

[Mike Meade, 2015-02-10 19:11:57]

mmeade@chromium.org changed reviewers:
+ maruel@google.com

[M-A Ruel, 2015-02-10 19:52:19]

maruel@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2015-02-10 19:52:19]

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:28: return getattr(obj, method)(*params)
I generally prefer to prefix all the rpc methods with something like "rpc_" or
anything similar. This makes it clear which methods are RPCs and secures the
interface.

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:64: self.stdout, self.stderr =
self.proc.communicate()
Note that you may like subprocess42.py, which supports streaming.
https://code.google.com/p/swarming/source/browse/utils/subprocess42.py?repo=c...

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:73: def Terminate(cls, index):
This index based access is surprising, especially that it means it's a memory
leak long term.

[Mike Meade, 2015-02-11 00:25:34]

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:28: return getattr(obj, method)(*params)
On 2015/02/10 19:52:19, M-A Ruel wrote:
> I generally prefer to prefix all the rpc methods with something like "rpc_" or
> anything similar. This makes it clear which methods are RPCs and secures the
> interface.

With SimpleXMLRPCServer any public method is considered an RPC method, and any
"protected" method is not exposed. In the actual client code these are accessed
via an rpc property, effectively making the calls client.rpc.Echo,
client.rpc.subprocess.Popen, etc. This seems to be very similar to what you are
proposing. Feel free to correct me if I misunderstood your comment.

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:64: self.stdout, self.stderr =
self.proc.communicate()
On 2015/02/10 19:52:19, M-A Ruel wrote:
> Note that you may like subprocess42.py, which supports streaming.
>
https://code.google.com/p/swarming/source/browse/utils/subprocess42.py?repo=c...

I ended up using subprocess42, but mainly for the added Windows support. The
problem is that the host controller is expected to run multiple subprocesses at
a time, and using the streaming means that they would need to implement the
reader threads on the host controller side. However if it proves to be a feature
request I can always add it since I am using subprocess42.

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:73: def Terminate(cls, index):
On 2015/02/10 19:52:19, M-A Ruel wrote:
> This index based access is surprising, especially that it means it's a memory
> leak long term.

Changed this to a dictionary, added a Delete method, and made the Terminate and
Kill methods delete the object. There is still a chance for a memory leak, but
at least the test case author is given multiple tools to manage this.

[Mike Meade, 2015-02-12 21:46:59]

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/1/testing/legion/client_rpc_me...
testing/legion/client_rpc_methods.py:64: self.stdout, self.stderr =
self.proc.communicate()
On 2015/02/11 00:25:34, Mike Meade wrote:
> On 2015/02/10 19:52:19, M-A Ruel wrote:
> > Note that you may like subprocess42.py, which supports streaming.
> >
>
https://code.google.com/p/swarming/source/browse/utils/subprocess42.py?repo=c...
> 
> I ended up using subprocess42, but mainly for the added Windows support. The
> problem is that the host controller is expected to run multiple subprocesses
at
> a time, and using the streaming means that they would need to implement the
> reader threads on the host controller side. However if it proves to be a
feature
> request I can always add it since I am using subprocess42.

The need for streaming reads actually came up pretty quickly. I added the
ability to do streaming reads and writes from the host controller without the
client controller blocking due to full pipes.

[M-A Ruel, 2015-02-12 22:05:25]

lgtm

https://codereview.chromium.org/870103005/diff/40001/testing/legion/client_rp...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/40001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:80: key = 'Process%d' % cls._process_count
The name _process_count is a bit misleading, since you do not reduce its value
in Delete(). It's more like process_next_id.

[Mike Meade, 2015-02-12 22:10:06]

New patchsets have been uploaded after l-g-t-m from maruel@chromium.org

[Mike Meade, 2015-02-12 22:12:47]

Can you take one more look. I forgot to upload my latest change. I added the
ability to do streaming reads from stderr and stdout. The client-based reader
thread uses subprocess42's yield_any method to "buffer" stdout and stderr, and
GetStdout and GetStderr will return the data already read. The buffers get reset
on a host-side read.

https://codereview.chromium.org/870103005/diff/40001/testing/legion/client_rp...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/40001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:80: key = 'Process%d' % cls._process_count
On 2015/02/12 22:05:25, M-A Ruel wrote:
> The name _process_count is a bit misleading, since you do not reduce its value
> in Delete(). It's more like process_next_id.

Done.

[M-A Ruel, 2015-02-12 22:16:11]

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:72: self.stdout_lock = threading.Lock()
one Lock would be enough.

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:115: def GetStdout(cls, key):
It's rare for a Get function to have side effect. It's acting like a Pop.

[Mike Meade, 2015-02-12 22:49:00]

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
File testing/legion/client_rpc_methods.py (right):

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:72: self.stdout_lock = threading.Lock()
On 2015/02/12 22:16:11, M-A Ruel wrote:
> one Lock would be enough.

Done.

https://codereview.chromium.org/870103005/diff/60001/testing/legion/client_rp...
testing/legion/client_rpc_methods.py:115: def GetStdout(cls, key):
On 2015/02/12 22:16:11, M-A Ruel wrote:
> It's rare for a Get function to have side effect. It's acting like a Pop.

Changed these to ReadStd*** methods and added usage documents explaining the
side effects.

[Mike Meade, 2015-02-13 17:32:49]

The CQ bit was checked by mmeade@chromium.org

[commit-bot: I haz the power, 2015-02-13 17:33:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870103005/80001

[commit-bot: I haz the power, 2015-02-13 18:36:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-13 18:36:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Mike Meade, 2015-02-17 16:57:02]

The CQ bit was checked by mmeade@chromium.org

[commit-bot: I haz the power, 2015-02-17 16:57:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/870103005/80001

[commit-bot: I haz the power, 2015-02-17 17:22:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-02-17 17:23:20]

Patchset 5 (id:??) landed as
https://crrev.com/759bbd2f8ecc15d79a67a7e368819ab422a10cb3
Cr-Commit-Position: refs/heads/master@{#316594}