Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eastl::shared_ptr<>::reset(): function not thread safe #378

Merged

Conversation

Wawha
Copy link
Contributor

@Wawha Wawha commented Jun 20, 2020

reset() function of shared_ptr is not thread safe.
It takes so many times to understand/find where come from that memory leak I have in my code. The problem was so difficult to reproduce and isolate!

So here the code:

	inline void ref_count_sp::release()
	{
		EASTL_ASSERT((mRefCount > 0) && (mWeakRefCount > 0));
		if(Internal::atomic_decrement(&mRefCount) > 0)
			Internal::atomic_decrement(&mWeakRefCount);
		else
		{
			free_value();

			if(Internal::atomic_decrement(&mWeakRefCount) == 0)
				free_ref_count_sp();
		}
	}

If 2 threads, with 2 shared_ptr<> pointed the same value, and are calling the release() function at the same time, the free_ref_count_sp(); could be not called.
It happens, when thread 1 is calling Internal::atomic_decrement(&mWeakRefCount); (due m_RefCount > 0) after the thread 2 call the code if(Internal::atomic_decrement(&mWeakRefCount) == 0). In that case free_ref_count_sp(); is not called, because on the thread 2, mWeakRefCount will have value 1.

The fix consist to remove that else condition and process each counter independently :

	inline void ref_count_sp::release()
	{
		EASTL_ASSERT((mRefCount > 0) && (mWeakRefCount > 0));
		if(Internal::atomic_decrement(&mRefCount) == 0)
			free_value();

		if(Internal::atomic_decrement(&mWeakRefCount) == 0)
			free_ref_count_sp();
	}

I try to write UTest for that case. For the moment, I used std::future and std::async because it's easier for me. If I have to used EAThread, could you tell me the best eastl function to used instead.

@Wawha Wawha marked this pull request as ready for review June 20, 2020 14:59
@Wawha Wawha force-pushed the shared-ptr-reset_not-thread-safe branch from ba53064 to 73c1f21 Compare June 20, 2020 15:06
@Wawha Wawha force-pushed the shared-ptr-reset_not-thread-safe branch from 73c1f21 to 53c0987 Compare June 20, 2020 15:44
@MaxEWinkler
Copy link
Contributor

Thank you so much for the fix.
This is a really good find and a definite issue.
Thanks again for taking the time to figure this out and sending us the patch :).

@MaxEWinkler MaxEWinkler merged commit fc6af13 into electronicarts:master Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants