You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/12/01 21:13:41 UTC
Re: Review Request 28052: Match future dispatch messages with type
info.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28052/#review63427
-----------------------------------------------------------
3rdparty/libprocess/include/process/gmock.hpp
<https://reviews.apache.org/r/28052/#comment105679>
We only checks `typeid` of a method right now. What if there are two methods having the same type but different name? That will cause `FUTURE_DISPATCH` or `DROP_DISPATCH` to capture/drop a method that is not expected.
- Jie Yu
On Nov. 20, 2014, 9 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28052/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 9 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2112
> https://issues.apache.org/jira/browse/MESOS-2112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The fix in this rb is to instead of matching pointer values, but to compare type_info from the member function pointers passed in. This ensures we're matching the same method.
>
>
> The downside of this approach is that, we no longer can do the following in tests:
>
> class A : ProcessBase<A> {
> public:
> virtual void foo() {};
> };
>
> class B : public A {
> public:
> virtual void foo() {};
> };
>
> Future<Nothing> future = FUTURE_DISPATCH(_, &B::foo);
>
> process.dispatch(B.self(), &A::foo);
>
> AWAIT_READY(future);
>
> We no longer can match the future on a derived method, but on dispatch passing the base member function pointer. This only used to work since the virtual offsets of both is matching the same address, but arguable is not the right behavior since the reverse is a false positive but will also match. We've done this in a few places in tests around the AllocatorProcess.
>
> Review: https://reviews.apache.org/r/28052
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4
> 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227
> 3rdparty/libprocess/include/process/event.hpp 294e2158876b25b06456a3619e547082d71e31ed
> 3rdparty/libprocess/include/process/gmock.hpp d6f2fc8fef8acd1498a36c86e09b28d986eefa15
> 3rdparty/libprocess/src/process.cpp 7a986d7defb03043ec70a48e161ede50deef9b26
>
> Diff: https://reviews.apache.org/r/28052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 28052: Match future dispatch messages with type
info.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 1, 2014, 8:13 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, lines 346-347
> > <https://reviews.apache.org/r/28052/diff/4/?file=771005#file771005line346>
> >
> > We only checks `typeid` of a method right now. What if there are two methods having the same type but different name? That will cause `FUTURE_DISPATCH` or `DROP_DISPATCH` to capture/drop a method that is not expected.
Do you have an example that could happen? From what I see in the type_info information the actual symbol name is used for matching, which contains the method name too.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28052/#review63427
-----------------------------------------------------------
On Nov. 20, 2014, 9 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28052/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 9 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2112
> https://issues.apache.org/jira/browse/MESOS-2112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The fix in this rb is to instead of matching pointer values, but to compare type_info from the member function pointers passed in. This ensures we're matching the same method.
>
>
> The downside of this approach is that, we no longer can do the following in tests:
>
> class A : ProcessBase<A> {
> public:
> virtual void foo() {};
> };
>
> class B : public A {
> public:
> virtual void foo() {};
> };
>
> Future<Nothing> future = FUTURE_DISPATCH(_, &B::foo);
>
> process.dispatch(B.self(), &A::foo);
>
> AWAIT_READY(future);
>
> We no longer can match the future on a derived method, but on dispatch passing the base member function pointer. This only used to work since the virtual offsets of both is matching the same address, but arguable is not the right behavior since the reverse is a false positive but will also match. We've done this in a few places in tests around the AllocatorProcess.
>
> Review: https://reviews.apache.org/r/28052
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4
> 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227
> 3rdparty/libprocess/include/process/event.hpp 294e2158876b25b06456a3619e547082d71e31ed
> 3rdparty/libprocess/include/process/gmock.hpp d6f2fc8fef8acd1498a36c86e09b28d986eefa15
> 3rdparty/libprocess/src/process.cpp 7a986d7defb03043ec70a48e161ede50deef9b26
>
> Diff: https://reviews.apache.org/r/28052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 28052: Match future dispatch messages with type
info.
Posted by Timothy Chen <tn...@apache.org>.
> On Dec. 1, 2014, 8:13 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, lines 346-347
> > <https://reviews.apache.org/r/28052/diff/4/?file=771005#file771005line346>
> >
> > We only checks `typeid` of a method right now. What if there are two methods having the same type but different name? That will cause `FUTURE_DISPATCH` or `DROP_DISPATCH` to capture/drop a method that is not expected.
>
> Timothy Chen wrote:
> Do you have an example that could happen? From what I see in the type_info information the actual symbol name is used for matching, which contains the method name too.
>
> Jie Yu wrote:
> Yeah, the review bot tests failed due to this issue https://reviews.apache.org/r/28570/
>
> Are you refering to type_info::name()? As far as I know, this is the type name not the symbol name:
> http://www.cplusplus.com/reference/typeinfo/type_info/name/
Hi Jie, it looks like it's failing on a EXPECT_CALL macro which is implemented directly by gmock.
My recent changes only touches FUTURE_DISPATCH and DROP_DISPATCH calls only, as we do the matching ourselves.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28052/#review63427
-----------------------------------------------------------
On Nov. 20, 2014, 9 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28052/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 9 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2112
> https://issues.apache.org/jira/browse/MESOS-2112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The fix in this rb is to instead of matching pointer values, but to compare type_info from the member function pointers passed in. This ensures we're matching the same method.
>
>
> The downside of this approach is that, we no longer can do the following in tests:
>
> class A : ProcessBase<A> {
> public:
> virtual void foo() {};
> };
>
> class B : public A {
> public:
> virtual void foo() {};
> };
>
> Future<Nothing> future = FUTURE_DISPATCH(_, &B::foo);
>
> process.dispatch(B.self(), &A::foo);
>
> AWAIT_READY(future);
>
> We no longer can match the future on a derived method, but on dispatch passing the base member function pointer. This only used to work since the virtual offsets of both is matching the same address, but arguable is not the right behavior since the reverse is a false positive but will also match. We've done this in a few places in tests around the AllocatorProcess.
>
> Review: https://reviews.apache.org/r/28052
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4
> 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227
> 3rdparty/libprocess/include/process/event.hpp 294e2158876b25b06456a3619e547082d71e31ed
> 3rdparty/libprocess/include/process/gmock.hpp d6f2fc8fef8acd1498a36c86e09b28d986eefa15
> 3rdparty/libprocess/src/process.cpp 7a986d7defb03043ec70a48e161ede50deef9b26
>
> Diff: https://reviews.apache.org/r/28052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 28052: Match future dispatch messages with type
info.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 1, 2014, 8:13 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, lines 346-347
> > <https://reviews.apache.org/r/28052/diff/4/?file=771005#file771005line346>
> >
> > We only checks `typeid` of a method right now. What if there are two methods having the same type but different name? That will cause `FUTURE_DISPATCH` or `DROP_DISPATCH` to capture/drop a method that is not expected.
>
> Timothy Chen wrote:
> Do you have an example that could happen? From what I see in the type_info information the actual symbol name is used for matching, which contains the method name too.
>
> Jie Yu wrote:
> Yeah, the review bot tests failed due to this issue https://reviews.apache.org/r/28570/
>
> Are you refering to type_info::name()? As far as I know, this is the type name not the symbol name:
> http://www.cplusplus.com/reference/typeinfo/type_info/name/
>
> Timothy Chen wrote:
> Hi Jie, it looks like it's failing on a EXPECT_CALL macro which is implemented directly by gmock.
> My recent changes only touches FUTURE_DISPATCH and DROP_DISPATCH calls only, as we do the matching ourselves.
Tim, that's because the DROP_DISPATCH() dropped a message that is not supposed to be dropped (they have the same signature but different name).
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28052/#review63427
-----------------------------------------------------------
On Nov. 20, 2014, 9 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28052/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 9 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2112
> https://issues.apache.org/jira/browse/MESOS-2112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The fix in this rb is to instead of matching pointer values, but to compare type_info from the member function pointers passed in. This ensures we're matching the same method.
>
>
> The downside of this approach is that, we no longer can do the following in tests:
>
> class A : ProcessBase<A> {
> public:
> virtual void foo() {};
> };
>
> class B : public A {
> public:
> virtual void foo() {};
> };
>
> Future<Nothing> future = FUTURE_DISPATCH(_, &B::foo);
>
> process.dispatch(B.self(), &A::foo);
>
> AWAIT_READY(future);
>
> We no longer can match the future on a derived method, but on dispatch passing the base member function pointer. This only used to work since the virtual offsets of both is matching the same address, but arguable is not the right behavior since the reverse is a false positive but will also match. We've done this in a few places in tests around the AllocatorProcess.
>
> Review: https://reviews.apache.org/r/28052
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4
> 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227
> 3rdparty/libprocess/include/process/event.hpp 294e2158876b25b06456a3619e547082d71e31ed
> 3rdparty/libprocess/include/process/gmock.hpp d6f2fc8fef8acd1498a36c86e09b28d986eefa15
> 3rdparty/libprocess/src/process.cpp 7a986d7defb03043ec70a48e161ede50deef9b26
>
> Diff: https://reviews.apache.org/r/28052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>
Re: Review Request 28052: Match future dispatch messages with type
info.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 1, 2014, 8:13 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/include/process/gmock.hpp, lines 346-347
> > <https://reviews.apache.org/r/28052/diff/4/?file=771005#file771005line346>
> >
> > We only checks `typeid` of a method right now. What if there are two methods having the same type but different name? That will cause `FUTURE_DISPATCH` or `DROP_DISPATCH` to capture/drop a method that is not expected.
>
> Timothy Chen wrote:
> Do you have an example that could happen? From what I see in the type_info information the actual symbol name is used for matching, which contains the method name too.
Yeah, the review bot tests failed due to this issue https://reviews.apache.org/r/28570/
Are you refering to type_info::name()? As far as I know, this is the type name not the symbol name:
http://www.cplusplus.com/reference/typeinfo/type_info/name/
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28052/#review63427
-----------------------------------------------------------
On Nov. 20, 2014, 9 a.m., Timothy Chen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28052/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2014, 9 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-2112
> https://issues.apache.org/jira/browse/MESOS-2112
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> The fix in this rb is to instead of matching pointer values, but to compare type_info from the member function pointers passed in. This ensures we're matching the same method.
>
>
> The downside of this approach is that, we no longer can do the following in tests:
>
> class A : ProcessBase<A> {
> public:
> virtual void foo() {};
> };
>
> class B : public A {
> public:
> virtual void foo() {};
> };
>
> Future<Nothing> future = FUTURE_DISPATCH(_, &B::foo);
>
> process.dispatch(B.self(), &A::foo);
>
> AWAIT_READY(future);
>
> We no longer can match the future on a derived method, but on dispatch passing the base member function pointer. This only used to work since the virtual offsets of both is matching the same address, but arguable is not the right behavior since the reverse is a false positive but will also match. We've done this in a few places in tests around the AllocatorProcess.
>
> Review: https://reviews.apache.org/r/28052
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/c++11/dispatch.hpp 76da2828cf36b6143d627dac66f3e0cc4416bae4
> 3rdparty/libprocess/include/process/dispatch.hpp bceda2a2ae8963921e8e19660243a8644feab227
> 3rdparty/libprocess/include/process/event.hpp 294e2158876b25b06456a3619e547082d71e31ed
> 3rdparty/libprocess/include/process/gmock.hpp d6f2fc8fef8acd1498a36c86e09b28d986eefa15
> 3rdparty/libprocess/src/process.cpp 7a986d7defb03043ec70a48e161ede50deef9b26
>
> Diff: https://reviews.apache.org/r/28052/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Timothy Chen
>
>