> On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 53-75 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line53> > > > > What's the motivation of storing this? Should the caller already have > > those information? > > Marco Massenzio wrote: > not necessarily - please note that this is executed asynchronously, so > the caller may have several of them running concurrently and having the > invocation returned together with the invocation will greatly simplifly the > caller's code. > > The whole point of this patch is to make the caller's API less > "low-level" and simplify the life of those using this facility. > > I have, for example, a module that executes commands upon an HTTP request > - having the `invocation` returned to me with the result, greatly simplifies > my code and enables me to return a Response without waiting for execution. > > Jie Yu wrote: > From what I can tell from the code you pasted below, doesn't seem to > simplify the caller's code *a lot*.
Point well taken, but I'm confused here, Jie - on the one hand, you here seem to be unhappy it does not simplify enough, then below you suggest it simplifies too much. The point here is - BenH and I went over the current usages of `subprocess()` and saw a common pattern, and agreed on a way that would enable us to (a) cover 99% of usage patterns and (b) avoid the same boilerplate code to be repeated over and over again. Three months on, and countless reviews after, this is what we arrived at: it would be good, at some point in time, to accept that this is **one** way of doing things, among countless others - and I'd like this code committed. Unless we all agree that it's either buggy/flawed/wrong, in which case, I'm happy to discard this patch and re-start from scratch. > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 119-130 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line119> > > > > Why not just use a single `int status` field here. The users can use > > WEXITSTATUS ... to derive those information themself. This is also > > consistent with other places in the code place. > > Marco Massenzio wrote: > `Subprocess` does exactly that (actually, it uses an `Option<int>`) - > this leaves the caller with the burned to do all the legwork: again and again > and again - it's all over the code base. > > The point of this patch is to encapsulate that work, having it done > (hopefully, properly) in **one** place and avoid to have the same code > sprinkled all over the codebase (and you can tell, most places, by copy & > paste). > > Jie Yu wrote: > What if there's no returnCode (due to signal)? Should you use Option<int> > returnCode here? Similarly, should you use Option<int> for signal as well. > What will the client code be look like? Do you still need to check if > (returnCode.isSome()) ... if (signal.isSome()) ... > > Also > 1) what if I want to know if WCOREDUMP is true or not, do you need to add > another boolean? > 2) what if the reaper cannot reap the subprocess (i.e., we cannot get the > exit status)? Please see the usage in the tests (this same patch). `returnCode` is EXIT_FAILURE and `signal` is non-zero - in case one cares (which most folks don't seems to, but whatever). if we can't get the exit status (`Subprocess::status` is `None()`) we return a `Failure` on the returned future and an error message (which is what ultimately is done practically everywhere `subprocess()` is used - while I guess I could've dreamt up tens of different scenarios, usage is pretty consistent in Mesos codebase and is what we're trying to encapsulate here). Note that `Subprocess::status` is still available to callers. > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, line 328 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line328> > > > > I don't like the name 'execute'. When you create the Subprocess > > instance, the subprocss is already launched and exec'ed. This is rather > > waiting for the subprocess to terminate. > > Marco Massenzio wrote: > This method most definitely does **not** wait. > > This is how one can use it as a caller (code simplified): > ``` > auto s = process::subprocess(commandInfo.command(), args); > > if (s.isError()) { > LOG(ERROR) << "Could not spawn subprocess: " << s.error(); > return http::ServiceUnavailable(s.error()); > } > > store(s.get().pid()); // <-- needed to reconcile with GETs > > Future<CommandResult> result_ = s->execute(); > result_.then([](const Future<CommandResult> &future) { > if (future.isFailed()) { > // mark the command as failed > return Nothing(); > } > auto result = future.get(); > // update status of job - use pid(); something equivalent to: > LOG(INFO) << "Result of '" << result.invocation.command << "'was: > " > << result.stdout(); > return Nothing(); > }).after(Seconds(30), [s](const Future<Nothing> &future) { > // update status of job to timed out; use `invocation` and > `stdout`. > s.get().cleanup(); > return Nothing(); > }); > > http::Response response = http::OK("{\"result\": \"OK\", \"pid\": \"" + > stringify(s.get().pid()) + "\"}"); > response.headers["Content-Type"] = "application/json"; > return response; > ``` > > Jie Yu wrote: > From the code above, can you just caputure commandInfo.command() in the > lambda and print it? > > ``` > string command = commandInfo.command(); > > result_.then([command](...) { > ... > LOG(INFO) << command << "..."; > }); > ``` > > Jie Yu wrote: > ALso, `auto s = process::subprocess(commandInfo.command(), args);` this > line fork and exec the subprocess. So having another `s->execute()` sounds > very confusing to me. I don't disagree - what would you suggest instead? (note about the example above: it's contrived - one can also imagine storing the Future in a map keyed by the id, and retrieve the outcome upon receiving a GET requests on the pid status; there may be other scenarios where just passing the commandInfo and/or the args or whatever in the lambda may be less desirable -- but, again, this is *one* way of doing things, not necessarily unique, and admittedly maybe not even *the* best). > On Nov. 20, 2015, 10:12 p.m., Jie Yu wrote: > > 3rdparty/libprocess/include/process/subprocess.hpp, lines 336-343 > > <https://reviews.apache.org/r/37336/diff/11/?file=1122402#file1122402line336> > > > > Why introduce this method? I think the caller should be responsible for > > killing the subprocess if needed. > > > > Also, os::killtree is in general not reliable and shouldn't be used in > > library code. > > Marco Massenzio wrote: > again, this is all about hiding implementation details from the caller > and make their life easier. > thanks for heads-up about `killtree()` - what would you suggest we use > instead? > > when you say "unreliable" what do you mean, exactly? > that it may fail to actually kill the process or that it risks blowing up > the app or segfaulting or whatever? > > Jie Yu wrote: > I guess I don't understand in what scenario should you issue killtree? > > Why it's not reliable? What if a process reparents to init? What if a > process calls setsid to escape from a session? well, I'd ratherr have something that works 99.9% of the cases in which it tries to clean up after itself, than nothing - obviously, if the process tries to do something clever to escape / avoid termination, I'm really not sure what else could one do about it? Again - what is your alternative suggestion? In the apidoc I was being rather explicit about the "no-guarantees" nature of this method: ``` the caller may want to try and clean up the process. ``` we give no refunds for failures to cleanup :) (we can of course be even more explicit about the issues you mentioned). - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37336/#review107421 ----------------------------------------------------------- On Nov. 10, 2015, 8:51 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37336/ > ----------------------------------------------------------- > > (Updated Nov. 10, 2015, 8:51 p.m.) > > > Review request for mesos, Joris Van Remoortere and Michael Park. > > > Bugs: MESOS-3035 > https://issues.apache.org/jira/browse/MESOS-3035 > > > Repository: mesos > > > Description > ------- > > The original API for `process::Subprocess` still left a lot of legwork > to do for the caller; we have now added an `execute()` method > that returns a `Future<Subprocess::Result>`. > > `Subprocess::Result`, also introduced with this patch, contains useful > information > about the command invocation (an `Invocation` struct); the exit code; > `stdout`; > and, optionally, `stderr` too. > > Once the Future completes, if successful, the caller will be able to retrieve > stdout/stderr; whether the command was successful; and whether it received a > signal > > > Diffs > ----- > > 3rdparty/libprocess/include/process/subprocess.hpp > f17816e813d5efce1d3bb1ff1e1111850eeda3ba > 3rdparty/libprocess/src/subprocess.cpp > efe0018d0414c4137fd833c153eb262232e712bc > 3rdparty/libprocess/src/tests/subprocess_tests.cpp > ac600a551fb1a7782ff33cce204b7819497ef54a > > Diff: https://reviews.apache.org/r/37336/diff/ > > > Testing > ------- > > make check > > (also tested functionality with an anonymous module that exposes an > `/execute` endpoint and runs arbitrary commands, asynchronously, > on an Agent) > > > Thanks, > > Marco Massenzio > >
