> On June 21, 2017, 11:42 a.m., Joseph Wu wrote: > > I agree this makes the finalization logic safer, but I don't see a way to > > add a `nullptr` to the `ProcessManager::processes` map. > > > > `ProcessManager::spawn` is the only location where the `processes` map is > > inserted, and this method CHECK-fails if you give it a `nullptr`. Two > > other locations use the map's `operator[]`, but those are guarded by a > > mutex so they will never empty-initialize a map entry. > > > > Can you note how you ran into a segfault here? It's more likely that the > > segfault was caused by a dereferencing a non-`nullptr` process that had > > already been deallocated (which would not be fixed by this review). > > Jiang Yan Xu wrote: > The use case is something like this: > > https://github.com/apache/mesos/blob/65152413836b58d01ace3a40bdc9056f9a489c6b/3rdparty/libprocess/src/tests/process_tests.cpp#L691 > > Arguably I made an error in doing: > > ``` > ProcessBase process; > UPID pid = spawn(&process, true); > > ... > > terminate(process); > wait(process); > ``` > > And the process (in a test method) goes out of scope before libprocess > finalize() so it segfaulted. > > Just felt it's not really necessary to fail (and it used to not) in this > way? > > Joseph Wu wrote: > I see. That error will still segfault, even with this patch. > > --- > > The tests (and otherwise invalid/risky uses of libprocess methods) did > not segfault in the past because we didn't clean up libprocess in the past.
Hmm you are right it's true that this wouldn't help. Discarding this. It'll probably not be much safer without using `shared_ptr`s. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60252/#review178522 ----------------------------------------------------------- On June 20, 2017, 4:16 p.m., Jiang Yan Xu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60252/ > ----------------------------------------------------------- > > (Updated June 20, 2017, 4:16 p.m.) > > > Review request for mesos and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > We don't need and shouldn't assume pointers in `processes` are > non-nullptr. > > > Diffs > ----- > > 3rdparty/libprocess/src/process.cpp > 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d > > > Diff: https://reviews.apache.org/r/60252/diff/1/ > > > Testing > ------- > > make check. > > > Thanks, > > Jiang Yan Xu > >
