Issue with mock with shared_ptr used in self-invocation

[ericlemes]

I've created a test to capture this issue, but didn't find out how to fix yet.

	class ISomeInterface3 {
	public:
		virtual ~ISomeInterface3() { }
		
		virtual void methodWithSomeSharedPointer(std::shared_ptr<ISomeInterface3> listener) = 0;
	};

	void production_shared_ptr_mock_used_in_invocation() {		
		Mock<ISomeInterface3> mockDevAssertListener;
		std::shared_ptr<ISomeInterface3> devAssertListener = std::shared_ptr<ISomeInterface3>(&mockDevAssertListener.get());

		fakeit::Fake(Dtor(mockDevAssertListener));						
		fakeit::Fake(Method(mockDevAssertListener, methodWithSomeSharedPointer));
						
		devAssertListener->methodWithSomeSharedPointer(devAssertListener);
		devAssertListener = nullptr;
	}

I think this issue happens because some of the invocations store the shared_ptr and because of the order things are destructed it calls the destructor after the internal mock object is killed.

[eranpeer]

That was a great help. I think I resolved this issue. I've just pushed the fix. Thanks,

…

On Mon, Aug 13, 2018 at 4:32 AM Eric Lemes ***@***.***> wrote: I've created a test to capture this issue, but didn't find out how to fix yet. class ISomeInterface3 { public: virtual ~ISomeInterface3() { } virtual void methodWithSomeSharedPointer(std::shared_ptr<ISomeInterface3> listener) = 0; }; void production_shared_ptr_mock_used_in_invocation() { Mock<ISomeInterface3> mockDevAssertListener; std::shared_ptr<ISomeInterface3> devAssertListener = std::shared_ptr<ISomeInterface3>(&mockDevAssertListener.get()); fakeit::Fake(Dtor(mockDevAssertListener)); fakeit::Fake(Method(mockDevAssertListener, methodWithSomeSharedPointer)); devAssertListener->methodWithSomeSharedPointer(devAssertListener); devAssertListener = nullptr; } I think this issue happens because some of the invocations store the shared_ptr and because of the order things are destructed it calls the destructor after the internal mock object is killed. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#153>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACc8gpZ4Dq6ysldXM4cX0cI0TAwSwgcnks5uQWO1gaJpZM4V6Vv5> .

-- Eran 1-424-2504000

[ericlemes]

Good to know that helped. I was struggling to fix it, since I don't understand the code very well. I'll get there eventually.

If the test pass, it is definitely fixed.

[eranpeer]

I’ve just noticed that the build is broken on master. It is my mistake. I assumed that it will compile on Linux-gcc. I was wrong. Working on it now. Sent from Mail for Windows 10 From: Eric Lemes Sent: Wednesday, August 15, 2018 12:06 AM To: eranpeer/FakeIt Cc: eranpeer; Comment Subject: Re: [eranpeer/FakeIt] Issue with mock with shared_ptr used inself-invocation (#153) Good to know that helped. I was struggling to fix it, since I don't understand the code very well. I'll get there eventually. If the test pass, it is definitely fixed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[eranpeer]

I fixed it!. I added a new api: mock.getShared() that returns a shared_ptr of the mock instance. It is like mock.get() but it returns a shared_ptr. Since fakeit is the owner of the mock instance, any attempt to wrap it with a shared_ptr will result in the shared_ptr deleting the instance when the reference count is 0. in order to avoid that simply obtain the shared_ptr from the mock.getShared() new api: class ISomeInterface3 { public: virtual ~ISomeInterface3() {} virtual void methodWithSomeSharedPointer(std::shared_ptr<ISomeInterface3> listener) = 0; }; void production_shared_ptr_mock_used_in_invocation() { std::shared_ptr<ISomeInterface3> pMockInstance; Mock<ISomeInterface3> mock; fakeit::Fake(Dtor(mock)); fakeit::Fake(Method(mock, methodWithSomeSharedPointer)); pMockInstance = mock.getShared(); pMockInstance->methodWithSomeSharedPointer(pMockInstance); pMockInstance = nullptr; } I think I can solve it in another way without introducing a new API. Let me know what you think about this approach, maybe a better name for getShared()?

…

On Wed, Aug 15, 2018 at 12:14 AM Eran Pe'er ***@***.***> wrote: I’ve just noticed that the build is broken on master. It is my mistake. I assumed that it will compile on Linux-gcc. I was wrong. Working on it now. Sent from Mail <https://go.microsoft.com/fwlink/?LinkId=550986> for Windows 10 *From: *Eric Lemes ***@***.***> *Sent: *Wednesday, August 15, 2018 12:06 AM *To: *eranpeer/FakeIt ***@***.***> *Cc: *eranpeer ***@***.***>; Comment ***@***.***> *Subject: *Re: [eranpeer/FakeIt] Issue with mock with shared_ptr used inself-invocation (#153) Good to know that helped. I was struggling to fix it, since I don't understand the code very well. I'll get there eventually. If the test pass, it is definitely fixed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACc8gn4vPhSOKjftJU728FkrcCBATJouks5uQ8h9gaJpZM4V6Vv5> .[image: https://github.com/notifications/beacon/ACc8gsjSGMl_tnycXhfm77OqWo2lEXR2ks5uQ8h9gaJpZM4V6Vv5.gif]

-- Eran 1-424-2504000

[ericlemes]

I like getShared, but to be honest, I didn't see the code yet. I'm going on holidays, back next Tuesday. I had a similar request from an user which is a getUnique and in the whole smart pointer world a "ReturnRef" for the When would be ideal. GMock has that. The idea is just returning a std::move, so it forces to use the move constructor instead of copy constructor which solves the problem of mocking methods that returns object from classes without copy constructor. AlwaysDo solves the problem, but it is not intuitive. These are just ideas and completely out of the scope of this issue.

…

On Thu, Aug 16, 2018, 05:39 eranpeer ***@***.***> wrote: I fixed it!. I added a new api: mock.getShared() that returns a shared_ptr of the mock instance. It is like mock.get() but it returns a shared_ptr. Since fakeit is the owner of the mock instance, any attempt to wrap it with a shared_ptr will result in the shared_ptr deleting the instance when the reference count is 0. in order to avoid that simply obtain the shared_ptr from the mock.getShared() new api: class ISomeInterface3 { public: virtual ~ISomeInterface3() {} virtual void methodWithSomeSharedPointer(std::shared_ptr<ISomeInterface3> listener) = 0; }; void production_shared_ptr_mock_used_in_invocation() { std::shared_ptr<ISomeInterface3> pMockInstance; Mock<ISomeInterface3> mock; fakeit::Fake(Dtor(mock)); fakeit::Fake(Method(mock, methodWithSomeSharedPointer)); pMockInstance = mock.getShared(); pMockInstance->methodWithSomeSharedPointer(pMockInstance); pMockInstance = nullptr; } I think I can solve it in another way without introducing a new API. Let me know what you think about this approach, maybe a better name for getShared()? On Wed, Aug 15, 2018 at 12:14 AM Eran Pe'er ***@***.***> wrote: > I’ve just noticed that the build is broken on master. It is my mistake. I > assumed that it will compile on Linux-gcc. I was wrong. Working on it now. > > > > > > Sent from Mail <https://go.microsoft.com/fwlink/?LinkId=550986> for > Windows 10 > > > > *From: *Eric Lemes ***@***.***> > *Sent: *Wednesday, August 15, 2018 12:06 AM > *To: *eranpeer/FakeIt ***@***.***> > *Cc: *eranpeer ***@***.***>; Comment ***@***.***> > *Subject: *Re: [eranpeer/FakeIt] Issue with mock with shared_ptr used > inself-invocation (#153) > > > > Good to know that helped. I was struggling to fix it, since I don't > understand the code very well. I'll get there eventually. > > If the test pass, it is definitely fixed. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#153 (comment)>, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ACc8gn4vPhSOKjftJU728FkrcCBATJouks5uQ8h9gaJpZM4V6Vv5 > > .[image: > https://github.com/notifications/beacon/ACc8gsjSGMl_tnycXhfm77OqWo2lEXR2ks5uQ8h9gaJpZM4V6Vv5.gif ] > > > -- Eran 1-424-2504000 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABsadOKEl8Y95BADp1BIgqkm74JAzMR8ks5uRPd_gaJpZM4V6Vv5> .

[eranpeer]

OK, I found a better way to fix it. No new API. your version of the test should pass now! I reverted the new API. On Wed, Aug 15, 2018 at 11:47 PM Eric Lemes <notifications@github.com> wrote:

…

I like getShared, but to be honest, I didn't see the code yet. I'm going on holidays, back next Tuesday. I had a similar request from an user which is a getUnique and in the whole smart pointer world a "ReturnRef" for the When would be ideal. GMock has that. The idea is just returning a std::move, so it forces to use the move constructor instead of copy constructor which solves the problem of mocking methods that returns object from classes without copy constructor. AlwaysDo solves the problem, but it is not intuitive. These are just ideas and completely out of the scope of this issue. On Thu, Aug 16, 2018, 05:39 eranpeer ***@***.***> wrote: > I fixed it!. I added a new api: mock.getShared() that returns a shared_ptr > of the mock instance. It is like mock.get() but it returns a shared_ptr. > Since fakeit is the owner of the mock instance, any attempt to wrap it with > a shared_ptr will result in the shared_ptr deleting the instance when the > reference count is 0. > in order to avoid that simply obtain the shared_ptr from the > mock.getShared() new api: > > class ISomeInterface3 { > public: > virtual ~ISomeInterface3() {} > > virtual void > methodWithSomeSharedPointer(std::shared_ptr<ISomeInterface3> listener) > = 0; > }; > > void production_shared_ptr_mock_used_in_invocation() { > std::shared_ptr<ISomeInterface3> pMockInstance; > Mock<ISomeInterface3> mock; > fakeit::Fake(Dtor(mock)); > fakeit::Fake(Method(mock, methodWithSomeSharedPointer)); > > pMockInstance = mock.getShared(); > pMockInstance->methodWithSomeSharedPointer(pMockInstance); > > pMockInstance = nullptr; > } > > I think I can solve it in another way without introducing a new API. Let me > know what you think about this approach, maybe a better name for > getShared()? > > On Wed, Aug 15, 2018 at 12:14 AM Eran Pe'er ***@***.***> wrote: > > > I’ve just noticed that the build is broken on master. It is my mistake. I > > assumed that it will compile on Linux-gcc. I was wrong. Working on it > now. > > > > > > > > > > > > Sent from Mail <https://go.microsoft.com/fwlink/?LinkId=550986> for > > Windows 10 > > > > > > > > *From: *Eric Lemes ***@***.***> > > *Sent: *Wednesday, August 15, 2018 12:06 AM > > *To: *eranpeer/FakeIt ***@***.***> > > *Cc: *eranpeer ***@***.***>; Comment ***@***.*** > > > *Subject: *Re: [eranpeer/FakeIt] Issue with mock with shared_ptr used > > inself-invocation (#153) > > > > > > > > Good to know that helped. I was struggling to fix it, since I don't > > understand the code very well. I'll get there eventually. > > > > If the test pass, it is definitely fixed. > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#153 (comment) >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/ACc8gn4vPhSOKjftJU728FkrcCBATJouks5uQ8h9gaJpZM4V6Vv5 > > > > .[image: > > > https://github.com/notifications/beacon/ACc8gsjSGMl_tnycXhfm77OqWo2lEXR2ks5uQ8h9gaJpZM4V6Vv5.gif > ] > > > > > > > > > -- > Eran > 1-424-2504000 > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#153 (comment)>, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABsadOKEl8Y95BADp1BIgqkm74JAzMR8ks5uRPd_gaJpZM4V6Vv5 > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#153 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACc8gpooD_ahFNKMFL8lFMDvdhXAhtloks5uRRWJgaJpZM4V6Vv5> .

-- Eran 1-424-2504000

[helmesjo]

Should this be closed?

[VirtualTim]

@helmesjo it looks like this code is currently still commented out: 19bd30a.

[FranckRJ]

It's intended as far as I understand, it's not necessary anymore.

[FranckRJ]

Because it look like it's fixed (cannot reproduce) I'll close it.