Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix unchecked_map destructor. #3723

Merged
merged 1 commit into from
Feb 8, 2022

Conversation

clemahieu
Copy link
Contributor

Fix an issue with unchecked_map::~unchecked_map where the condition variable signal can be missed since the object mutex isn't acquired while setting stopped=true.

@clemahieu clemahieu added the bug label Feb 8, 2022
@clemahieu clemahieu added this to the V24.0 milestone Feb 8, 2022
@theohax
Copy link
Contributor

theohax commented Feb 8, 2022

In practice I doubt this can actually bite us, but yeah, the proper way of announcing changes via CVs is under the mutex indeed, even if the change is atomic like this one. Just curious, have you seen this cause issues?

theohax
theohax previously approved these changes Feb 8, 2022
@dsiganos
Copy link
Contributor

dsiganos commented Feb 8, 2022

@theohax In practice, it would bite us on node shutdown, the node could deadlock on shutdown.

@clemahieu
Copy link
Contributor Author

If the working thread is between these two lines when stopped is set to true and the condition variable in notified, it will neither detect the stopped flag nor detect the condition variable notification.

	while (!stopped)
	{
		if (!buffer.empty ())

dsiganos
dsiganos previously approved these changes Feb 8, 2022
thsfs
thsfs previously approved these changes Feb 8, 2022
…ariable signal can be missed since the object mutex isn't acquired while setting stopped=true. Converts unchecked_map::stopped to a non-atomic bool since it needs a mutex anyway.
@clemahieu clemahieu merged commit 7ea11f8 into nanocurrency:develop Feb 8, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants