The continuation needs to go through the process manager once again to obtain a reference to the process that we'd like to deliver the message to. One thing we could have done to avoid capturing 'this' is to grab the ProcessReference early and pass it through to the continuation. Unfortunately the assumption is that ProcessReferences are used quickly after being obtained, since the termination code spins on this:
https://github.com/apache/mesos/blob/c89f571c7c81048ca83dc7098e35e847281ff672/3rdparty/libprocess/src/process.cpp#L3117-L3122 Another approach is to extend 'this' explicitly, benh has proposed something like a "defer to 'this'" to me recently which would make deferring to this safe, but it requires that the class extend from enable_shared_from_this and will make us use shared pointers. However, this newly introduced code which captures 'this' tries to fit in with the existing pattern of libprocess finalization, where we need to ensure that no further accesses to the process manager will occur before we delete it (we don't leak the global object AFAIK): https://github.com/apache/mesos/blob/c89f571c7c81048ca83dc7098e35e847281ff672/3rdparty/libprocess/src/process.cpp#L1189-L1252 Here we assume that finalization will shutdown all of the sockets before deleting the process manager, which is the same assumption that is made in the socket recv loops that access the process manager: https://github.com/apache/mesos/blob/c89f571c7c81048ca83dc7098e35e847281ff672/3rdparty/libprocess/src/process.cpp#L802 We could have written it without a 'this' capture and instead use the global 'process_manager', and then it becomes logically equivalent to the approach in the recv loops: parse(*request) .onAny([socket, request](const Future<Message*>& message) { ... process_manager->deliver(...); // instead of // deliver(...); ... } Note that there are additional accesses to process_manager as well that need to be thought through in terms of finalization for any solution to finalization. If you have some ideas to make the finalization more robust that would be great! On Tue, Nov 22, 2016 at 2:02 AM, Benjamin Bannier < [email protected]> wrote: > Hi, > > just came across this with our `mesos-this-capture` clang-tidy check: > > > + // It is guaranteed that the continuation would run before the next > > + // request arrives. Also, it's fine to pass the `this` pointer to > the > > + // continuation as this would get executed synchronously (if still > pending) > > + // from `SocketManager::finalize()` due to it closing all active > sockets > > + // during libprocess finalization. > > + parse(*request) > > + .onAny([this, socket, request](const Future<Message*>& future) { > > Even though there is a comment hinting that capturing `this` here should be > safe, I am not sure this is a maintainable solution, e.g., still working > if we > begin to manage the lifetime of `process_manager` instead of simple > leaking a > global object. Above code would continue to compile in that case, but > become racy. > > Is there some actor we could dispatch to to make this safe, or do we need > a new abstraction? > > > Cheers, > > Benjamin > >
