Well, why would they be extending the lifetime of a temporary if they don't understand the semantics in the first place? :)
They should understand the difference between: const T& t = foo(); // Returns T. const T& t = option.get(); // Ok. const T& t = (temporaryOption()).get(); // Bad! This pattern is not unique to our primitives, for example, consider: const T& t = (temporaryMap()).at(0); // Bad! It just so happens that in the case of Option, we used to return copies for simplicity where now we return references for efficiency so it's important to understand these new semantics when using Try/Result/Option. This is akin to not using things like std::map::at or std::vector::[] on a temporary object. On Thu, Feb 27, 2014 at 2:05 PM, Dominic Hamon <[email protected]> wrote: > > > > On Thu, Feb 27, 2014 at 2:02 PM, Ben Mahler <[email protected]>wrote: > >> >> >> > On Feb. 27, 2014, 7:09 p.m., Ben Mahler wrote: >> > > Hey Dominic, I found an issue applying these Option patches, looks >> like slave.cpp:3001 ( >> https://github.com/apache/mesos/blob/20a6993e1b50bf46089cd5e059634ac446b08ac2/src/slave/slave.cpp#L3001) >> is problematic: >> > > >> > > struct ExecutorState >> > > { >> > > ExecutorID id; >> > > Option<ExecutorInfo> info; >> > > Option<ContainerID> latest; >> > > hashmap<ContainerID, RunState> runs; >> > > unsigned int errors; >> > > }; >> > > >> > > void Framework::recoverExecutor(const ExecutorState& state) >> > > { >> > > ... >> > > const RunState& run = state.runs.get(latest).get(); >> > > <----- rvalue -------> >> > > < lvalue of temp Option::t > >> > > } >> > > >> > > I'm not 100% clear on what the standard specifies here, but it >> doesn't look like we're extending the life of a temporary here, since we're >> grabbing a _member_ from a temporary. >> > > >> > > [ RUN ] SlaveRecoveryTest/0.RecoverStatusUpdateManager >> > > Detaching after fork from child process 51693. >> > > Detaching after fork from child process 51694. >> > > Detaching after fork from child process 51708. >> > > Detaching after fork from child process 51709. >> > > WARNING: Logging before InitGoogleLogging() is written to STDERR >> > > I0227 18:46:06.952373 51709 exec.cpp:131] Version: 0.19.0 >> > > I0227 18:46:06.956939 51742 exec.cpp:205] Executor registered on >> slave 2014-02-27-18:46:06-1740121354-59276-51387-0 >> > > Registered executor on smfd-bkq-03-sr4.devel.twitter.com >> > > Starting task 9fe870d9-4b47-489f-b863-2a929ade31a8 >> > > sh -c 'sleep 1000' >> > > Forked command at 51749 >> > > >> > > Program received signal SIGSEGV, Segmentation fault. >> > > [Switching to Thread 0x7fffecb03940 (LWP 51403)] >> > > 0x0000000000a09656 in >> boost::unordered::detail::table<boost::unordered::detail::map<std::allocator<std::pair<mesos::TaskID >> const, mesos::internal::slave::state::TaskState> >, mesos::TaskID, >> mesos::internal::slave::state::TaskState, boost::hash<mesos::TaskID>, >> std::equal_to<mesos::TaskID> > >::begin (this=0x7fffd803d810) at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/unordered/detail/table.hpp:249 >> > > 249 return size_ ? iterator(get_previous_start()->next_) >> : iterator(); >> > > (gdb) where >> > > #0 0x0000000000a09656 in >> boost::unordered::detail::table<boost::unordered::detail::map<std::allocator<std::pair<mesos::TaskID >> const, mesos::internal::slave::state::TaskState> >, mesos::TaskID, >> mesos::internal::slave::state::TaskState, boost::hash<mesos::TaskID>, >> std::equal_to<mesos::TaskID> > >::begin (this=0x7fffd803d810) at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/unordered/detail/table.hpp:249 >> > > #1 0x00007ffff62deccb in >> boost::unordered::unordered_map<mesos::TaskID, >> mesos::internal::slave::state::TaskState, boost::hash<mesos::TaskID>, >> std::equal_to<mesos::TaskID>, std::allocator<std::pair<mesos::TaskID const, >> mesos::internal::slave::state::TaskState> > >::begin (this=0x7fffd803d810) >> at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/unordered/unordered_map.hpp:209 >> > > #2 0x00007ffff62decf7 in >> boost::range_detail::range_begin<hashmap<mesos::TaskID, >> mesos::internal::slave::state::TaskState> const> (c=...) >> > > at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/range/begin.hpp:49 >> > > #3 0x00007ffff62ded0f in >> boost::range_adl_barrier::begin<hashmap<mesos::TaskID, >> mesos::internal::slave::state::TaskState> > (r=...) at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/range/begin.hpp:119 >> > > #4 0x00007ffff62ded37 in >> boost::foreach_detail_::begin<hashmap<mesos::TaskID, >> mesos::internal::slave::state::TaskState>, mpl_::bool_<true> > (col=...) >> > > at >> ../3rdparty/libprocess/3rdparty/boost-1.53.0/boost/foreach.hpp:674 >> > > #5 0x00007ffff62a4868 in >> mesos::internal::slave::Framework::recoverExecutor (this=0x7fffd804d6c0, >> state=...) at ../../src/slave/slave.cpp:3025 >> > > #6 0x00007ffff62aa668 in >> mesos::internal::slave::Slave::recoverFramework (this=0x7fffdc01c230, >> state=...) at ../../src/slave/slave.cpp:2767 >> > > #7 0x00007ffff62ab115 in mesos::internal::slave::Slave::recover >> (this=0x7fffdc01c230, _state=...) at ../../src/slave/slave.cpp:2576 >> > > #8 0x00007ffff62c77c4 in std::tr1::_Mem_fn<process::Future<Nothing> >> (mesos::internal::slave::Slave::*)(Result<mesos::internal::slave::state::SlaveState> >> const&)>::operator() (this=0x7fffe0036a40, >> > > __object=0x7fffdc01c230, __a1=...) at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/functional_iterate.h:214 >> > > #9 0x00007ffff62cbdb1 in >> std::tr1::_Bind<std::tr1::_Mem_fn<process::Future<Nothing> >> (mesos::internal::slave::Slave::*)(const >> Result<mesos::internal::slave::state::SlaveState>&)> >> ()(std::tr1::_Placeholder<1>, >> Result<mesos::internal::slave::state::SlaveState>)>::operator()<mesos::internal::slave::Slave*>(mesos::internal::slave::Slave >> *&) (this=0x7fffe0036a40, __u1=@0x7fffecb02d40) >> > > at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/bind_iterate.h:45 >> > > #10 0x00007ffff62cbdf1 in >> std::tr1::_Function_handler<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*),std::tr1::_Bind<std::tr1::_Mem_fn<process::Future<Nothing> >> (mesos::internal::slave::Slave::*)(const >> Result<mesos::internal::slave::state::SlaveState>&)> >> ()(std::tr1::_Placeholder<1>, >> Result<mesos::internal::slave::state::SlaveState>)> >::_M_invoke(const >> std::tr1::_Any_data &, mesos::internal::slave::Slave *) (__functor=..., >> __a1=0x7fffdc01c230) at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/functional_iterate.h:488 >> > > #11 0x00007ffff62e1b4d in std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)>::operator()(mesos::internal::slave::Slave >> *) const (this=0x7fffe0036760, __a1=0x7fffdc01c230) >> > > at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/functional_iterate.h:868 >> > > #12 0x00007ffff62f8a5d in process::internal::pdispatcher<Nothing, >> mesos::internal::slave::Slave>(process::ProcessBase *, >> std::tr1::shared_ptr<std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)> >, >> std::tr1::shared_ptr<process::Promise<Nothing> >) (process=0x7fffdc01c590, >> thunk=std::tr1::shared_ptr (count 2) 0x7fffecb02e50, >> promise=std::tr1::shared_ptr (count -536638560) 0x7fffecb02e30) >> > > at ../../3rdparty/libprocess/include/process/dispatch.hpp:88 >> > > #13 0x00007ffff62eedd5 in std::tr1::_Bind<void (* >> ()(std::tr1::_Placeholder<1>, >> std::tr1::shared_ptr<std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)> >, >> std::tr1::shared_ptr<process::Promise<Nothing> >))(process::ProcessBase*, >> std::tr1::shared_ptr<std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)> >, >> std::tr1::shared_ptr<process::Promise<Nothing> >> >)>::operator()<process::ProcessBase*>(process::ProcessBase *&) >> (this=0x7fffe002d340, __u1=@0x7fffecb02ea0) at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/bind_iterate.h:45 >> > > #14 0x00007ffff62eee4d in std::tr1::_Function_handler<void >> ()(process::ProcessBase*),std::tr1::_Bind<void (* >> ()(std::tr1::_Placeholder<1>, >> std::tr1::shared_ptr<std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)> >, >> std::tr1::shared_ptr<process::Promise<Nothing> >))(process::ProcessBase*, >> std::tr1::shared_ptr<std::tr1::function<process::Future<Nothing> >> ()(mesos::internal::slave::Slave*)> >, >> std::tr1::shared_ptr<process::Promise<Nothing> >)> >::_M_invoke(const >> std::tr1::_Any_data &, process::ProcessBase *) (__functor=..., >> __a1=0x7fffdc01c590) >> > > at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/functional_iterate.h:502 >> > > #15 0x00007ffff66bcded in std::tr1::function<void >> ()(process::ProcessBase*)>::operator()(process::ProcessBase *) const >> (this=0x7fffe0035500, __a1=0x7fffdc01c590) >> > > at >> /usr/lib/gcc/x86_64-redhat-linux/4.1.2/../../../../include/c++/4.1.2/tr1/functional_iterate.h:868 >> > > #16 0x00007ffff6679593 in process::ProcessBase::visit >> (this=0x7fffdc01c590, event=...) at >> ../../../3rdparty/libprocess/src/process.cpp:3183 >> > > #17 0x00007ffff6690e3e in process::DispatchEvent::visit >> (this=0x7fffe0035580, visitor=0x7fffdc01c590) at >> ../../../3rdparty/libprocess/include/process/event.hpp:136 >> > > #18 0x000000000066b1fa in process::ProcessBase::serve >> (this=0x7fffdc01c590, event=...) at >> ../../3rdparty/libprocess/include/process/process.hpp:38 >> > > #19 0x00007ffff66840f4 in process::ProcessManager::resume >> (this=0x10c41d0, process=0x7fffdc01c590) at >> ../../../3rdparty/libprocess/src/process.cpp:2611 >> > > #20 0x00007ffff6684948 in process::schedule (arg=0x0) at >> ../../../3rdparty/libprocess/src/process.cpp:1305 >> > > #21 0x00007ffff4ffe83d in start_thread () from /lib64/libpthread.so.0 >> > > #22 0x00007ffff3d6526d in clone () from /lib64/libc.so.6 >> > >> > Dominic Hamon wrote: >> > If i read this right, the hashmap is returning a copy of an >> Option<T> and then we call get() on that which should be returning a const >> RunState&. There shouldn't be any need to extend any lifetime. >> > >> > If the hashmap was returning a reference to an Option, I can see >> the problem. What am I missing? >> > >> > Ben Mahler wrote: >> > Previously, we were binding to an rvalue RunState, as Option::get >> was returning a copy. The standard guarantees that the lifetime of this >> RunState temporary would be extended (8.5.3 of the standard). >> > >> > Now Option::get is returning an rvalue of a RunState&, this >> reference lives inside the intermediate temporary Option in the expression. >> The lifetime of this Option _must_ be extended for the RunState reference >> to remain valid. But, I don't think the standard ensure this since the >> Option rvalue is an intermediate part of the expression. 12.2 of the >> standard is interesting here, but it's important to note that get() is >> returning a _reference_, not an object. >> > >> > Dominic Hamon wrote: >> > the fix for the callsite is to put the Option into an explicit >> temporary as a copy, however this makes me uncomfortable as the error is a >> runtime one. Ie, it's not obvious to someone that writing it as written is >> invalid. >> > >> > Another option is to tighten up hashmap so that get returns the >> value by reference, not an Option, and it's illegal to call 'get' when the >> key doesn't exist. Similar to C++11's map::at() method. >> > >> > The final option is to not change the Option return to a reference. >> > >> > Ben Mahler wrote: >> > What about just making a copy in the caller? :) >> > >> > const RunState& run = state.runs.get(latest).get(); >> > >> > becomes: >> > >> > RunState run = state.runs.get(latest).get(); >> > const RunState run = state.runs.get(latest).get(); // If const >> semantics are strongly desired. >> > >> > Ben Mahler wrote: >> > But yeah, you're right, the fact that we're doing a >> CHECK(contains(...)) followed by a get() means we probably should just be >> storing the Option anyway :) >> >> I think the existing code here is a bit unfortunate in the first place, >> what about this: >> >> Before: >> CHECK(state.runs.contains(latest)) >> << "Cannot find latest run " << latest << " for executor " << >> state.id >> << " of framework " << id; >> >> const RunState& run = state.runs.get(latest).get(); // Yuck! >> >> After: >> const Option<RunState>& run = state.runs.get(latest); >> CHECK_SOME(run) >> << "Cannot find latest run " << latest << " for executor " << >> state.id >> << " of framework " << id; >> >> // Now just use 'run.get()' where 'run' was used previously. >> // Alternatively, if needed: >> // const RunState& run_ = run.get(); >> > > You'll find no argument from me that that's much cleaner, however how > would a developer know to write that code? I don't know many who really > understand reference lifetime extension semantics off the top of their > head. ;) > > > > >> >> >> - Ben >> >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/18386/#review35673 >> ----------------------------------------------------------- >> >> >> On Feb. 27, 2014, 5:12 p.m., Dominic Hamon wrote: >> > >> > ----------------------------------------------------------- >> > This is an automatically generated e-mail. To reply, visit: >> > https://reviews.apache.org/r/18386/ >> > ----------------------------------------------------------- >> > >> > (Updated Feb. 27, 2014, 5:12 p.m.) >> > >> > >> > Review request for mesos and Ben Mahler. >> > >> > >> > Bugs: MESOS-1008 >> > https://issues.apache.org/jira/browse/MESOS-1008 >> > >> > >> > Repository: mesos-git >> > >> > >> > Description >> > ------- >> > >> > See summary >> > >> > >> > Diffs >> > ----- >> > >> > src/linux/fs.hpp 1d86dd0d24c3daae957b5eec387638d1e8e6d7db >> > src/linux/fs.cpp e5f4f9a16becd4e5960d0cbb7f988736188b2426 >> > src/log/log.cpp 62dc9286285d5981aa5fc63e125d3c3f51d4f457 >> > src/sched/sched.cpp dcb3158d2acd93a107fda51b19e92899a092e628 >> > >> > Diff: https://reviews.apache.org/r/18386/diff/ >> > >> > >> > Testing >> > ------- >> > >> > make check >> > >> > 'grep' for cases where Options are reassigned after references are >> taken. >> > >> > >> > Thanks, >> > >> > Dominic Hamon >> > >> > >> >> > > > -- > Dominic Hamon | @mrdo | Twitter > *There are no bad ideas; only good ideas that go horribly wrong.* >
