> On Nov. 17, 2013, 8:53 p.m., Benjamin Hindman wrote: > > 3rdparty/libprocess/include/process/owned.hpp, line 143 > > <https://reviews.apache.org/r/15333/diff/1/?file=380731#file380731line143> > > > > IIUC, you're using 'shared' to protect against two threads/processes > > from simultaneously calling Owned::share? Why would two threads/processes > > be doing this in the first place (considering this is supposed to be > > "owned")? Even if this was the case, I'm not convinced you've got > > sufficient coverage. Consider: thread 1 context switches before the CAS in > > Owned::share (line 143) and thread 2 executes Owned::get and context > > switches before 'return data->t;' (line 106) then thread 1 resumes and does > > 'data.reset()' which means thread 2 is going to deference a NULL pointer > > when it resumes. Unless I'm missing something?
To answer your first question, we saw many places in the code that copy Owned pointers (i.e., two Owned pointers holding the same object). Therefore, I decided to provide the following semantics: 1) if there is only one Owned pointer holding the object, we should have no problem doing Owned::share() 2) if there are more than one Owned pointers holding the object, only one of them may successfully do Owned:shared(), other Owned's operations (e.g. ->, get(), etc.) should fail after the ownership has been transferred. In that case, we try to provide as many diagnosis information as possible such as a CHECK. In your case, the program won't crash because thread 2 is still holding the reference to Data object, therefore, it will not be deleted even if thread 1 calls 'data.reset()'. However, it's likely that data->t will be deleted by the Shared object (because it is out of scope later), and we are trying to dereference data->t through the Owned pointer in thread 2, which is bad. Probably what we can do is: instead of using a separate boolean flag, we use the data->t itself to distinguish if it is shared or not. In other words, if data->t is NULL, then it is shared, otherwise, it is not. Thoughts? - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15333/#review29028 ----------------------------------------------------------- On Nov. 8, 2013, 1:30 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15333/ > ----------------------------------------------------------- > > (Updated Nov. 8, 2013, 1:30 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > See summary. > > Renamed to Shared<T>::upgrade to Shared<T>::own(). > > Also, moved owned.hpp to libprocess. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/Makefile.am 47e8dbb > 3rdparty/libprocess/3rdparty/stout/include/stout/owned.hpp 3433f50 > 3rdparty/libprocess/Makefile.am d348780 > 3rdparty/libprocess/include/process/owned.hpp PRE-CREATION > 3rdparty/libprocess/include/process/shared.hpp e0c7991 > 3rdparty/libprocess/include/process/statistics.hpp fbae641 > 3rdparty/libprocess/src/tests/owned_tests.cpp PRE-CREATION > 3rdparty/libprocess/src/tests/shared_tests.cpp 860a9aa > 3rdparty/libprocess/src/tests/statistics_tests.cpp 8695f45 > src/master/master.hpp e377af8 > src/master/master.cpp 8e14a07 > src/sched/sched.cpp 3abe72f > src/slave/gc.hpp 083aa79 > src/slave/monitor.cpp 9cb6256 > src/slave/reaper.hpp 4498139 > src/slave/slave.hpp 68526f3 > src/tests/cluster.hpp bea395a > > Diff: https://reviews.apache.org/r/15333/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
