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

Added mutex unlockAll #17

Merged
merged 1 commit into from
Jun 1, 2018
Merged

Conversation

XeniaSiskaki
Copy link
Contributor

@XeniaSiskaki XeniaSiskaki commented Apr 28, 2018

Add possibility for user to release all locks for #14

@XeniaSiskaki XeniaSiskaki changed the title Added mutex unlockAll for #14 Added mutex unlockAll Apr 28, 2018
@ronkorving
Copy link
Collaborator

Thanks @XeniaSiskaki !

Sorry for being so nitpicky about naming, I hope it doesn't hurt your motivation in your contribution. Changing it later would constitute a breaking change however, so I wanna make sure we get it right on the first try.

I think unlockAll is not technically correct. The mutex can only be locked zero or one time. Its state is very boolean. Your implementation of unlockAll does what we agreed upon in #14, which implies it doesn't actually unlock the mutex. You still need to call unlock to do that. This is good, but that does mean that unlockAll to me is a bit confusing a name.

What it does is, it forgets all waiting callbacks, so that after a call to unlock they will not be invoked. That's why in #14 I suggested calling it reset, which arguably is also not a great name. Suggestions very welcome, but IMHO unlockAll is a little bit confusing. Thoughts?

@XeniaSiskaki
Copy link
Contributor Author

Hey, sorry for the late response, I was on vacation! I think I'll get to it this weekend, as I don't have much time on work days.

I agree that unlockAll doesn't describe what the function actually does. However, I still think reset is a bit vague as it doesn't explain what it resets. It's too general in my opinion. Maybe something like resetQueue would be more fitting?

@ronkorving
Copy link
Collaborator

Sure, that works for me 👍

@XeniaSiskaki
Copy link
Contributor Author

Sorry, took me longer to get back to it!

@ronkorving
Copy link
Collaborator

No worries, thanks for your contribution!

@ronkorving ronkorving merged commit b485119 into Wizcorp:master Jun 1, 2018
@XeniaSiskaki
Copy link
Contributor Author

@ronkorving when do you think this will make it into npm?

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.

2 participants