> On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/reaper.cpp, lines 31-34 > > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line31> > > > > Not your bug, but any reason we can't reuse an existing promise if it's > > already in the map?
It seems that two callers reaping the same pid should not be a supported use-case, given the semantics of process reaping. Now that I've changed the behavior, duplicate calls will result in duplicate calls to waitpid(pid), which is not the right thing to do. We could re-use the promise, however, this opens up a race: -> reap(pid); -> waitpid(pid) -> promises[pid] satisfied and deleted -> reap(pid) duplicate call arrives in the queue, but does not receive the exit status, as we've already reaped the pid Given this, I think we should not support two calls to reap against the same pid, so I've opted to return a Failure when this is detected instead. Sounds good? > On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/src/reaper.cpp, lines 36-37 > > <https://reviews.apache.org/r/17304/diff/1/?file=447748#file447748line36> > > > > Since we've moved this into a library let's kill this LOG or make it > > VLOG. It should be expected that a user of this would want to print > > something like that out if they got back a None. Same for the logging > > statements below. Good catch! > On Jan. 24, 2014, 7:40 a.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/reaper.hpp, line 40 > > <https://reviews.apache.org/r/17304/diff/1/?file=447747#file447747line40> > > > > As long as we're making this cleanup, how do you feel about > > s/monitor/reap/ and s/reap/wait/ below? Thanks for the poke! My plan was to expose this as: Future<Option<int> > process::reap(pid_t); And to use a static reaper object under the covers. Will update with this shortly. - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/17304/#review32704 ----------------------------------------------------------- On Jan. 24, 2014, 7:05 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/17304/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2014, 7:05 a.m.) > > > Review request for mesos, Benjamin Hindman, Ian Downes, and Jie Yu. > > > Bugs: MESOS-943 > https://issues.apache.org/jira/browse/MESOS-943 > > > Repository: mesos-git > > > Description > ------- > > This is a copy of the Reaper from Mesos, with one key difference: This now > only reaps pids that were monitored explicitly. > > The test needed updating to reflect the fact that we can now retrieve the > exit status for an already exited process. > > This also opens up the opportunity to eliminate the 1 second delay in the > Reaper (using threads or SIGCHLD), but this is a copy of the Mesos Reaper for > now. > > > Diffs > ----- > > 3rdparty/libprocess/Makefile.am 40f01a7b3803696ccca440c8326e1d6d7c377459 > 3rdparty/libprocess/include/process/reaper.hpp PRE-CREATION > 3rdparty/libprocess/src/reaper.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/reaper_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/17304/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
