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

Add error check to Mutex by default #8563

Conversation

@ysbaddaden
Copy link
Member

ysbaddaden commented Dec 6, 2019

Alternative to #8560 that implements deadlock protection and prevents bogus unlocks by default (raises on deadlock), while still allowing reentrant (optional) and unsafe (no error check).

See #8295 (comment)

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Dec 6, 2019

I would like to avoid the usage of type and unsafe terms.

Instead of type can we use protection or policy with maybe deadlock_detection, reentrant, non_reentrant/unprotected or similar as values?

@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 7, 2019

Why?

To me, the name must have unsafe in it to let people know that this is unsafe, and they have to be extra careful to avoid deadlocks. Type vs protection/policy seems arbitrary.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Dec 7, 2019

@RX14 the proposed "unsafe" behavior is totally valid and has its use cases. I think that calling it unsafe is unprecise on what are the guarantees offered.


Another alternative to type is kind it's still generic but I try to avoid type named properties. I found it ambiguous when describing it.

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

ysbaddaden commented Dec 7, 2019

Well, manipulating pointers is valid, unsafe but valid.

Using unchecked mutexes properly is fine, but it has dangerous/tricky semantics that developers must take care to avoid, without runtime protections. I mean: any fiber being capable to unlock the mutex is unsafe behavior to me.

Type is because pthreads' spec uses that term. It can be Kind. I'm not sure about Protection. Maybe Policy.

Default is because that's the default behavior and you should never have to specify it.

Reentrant was already used; I think I would have used Recursive otherwise.

@oprypin

This comment has been minimized.

Copy link
Contributor

oprypin commented Dec 7, 2019

I don't like default because there's no information behind it, specifically if someone were to read just code.
checked would be nice for the default.

As for the parameter name, policy seems fine, if type is somehow undesirable.

@asterite

This comment has been minimized.

Copy link
Member

asterite commented Dec 7, 2019

Does Golang have these two kind of mutexes? If not, could we maybe have a mutex internal to the runtime that's unsafe but fast, but always expose a safe mutex to users? From a user's perspective, it's not clear when to use reentrant or not. If we do know at runtime, let's keep that logic to ourselves.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Dec 7, 2019

kind / policy / behavior and options checked, reentrant, unchecked. (@oprypin 💡)

unsafe as a term is used a lot in crystal for low level stuff and I don't see this as one of them, and it could be read as nop/skip.

The main thing I want is to avoid the usage of type and unsafe terms.

@ysbaddaden

This comment has been minimized.

Copy link
Member Author

ysbaddaden commented Dec 7, 2019

@asterite Go mutexes don't associate the lock to any goroutine. I.e. they're unchecked, period. No reentrancy, no checks.

We only use reentrant for crystal once (for lazy initializers?) to avoid a potential deadlock, if I understand correctly. Thought I think a recursive initializer would probably recurse infinitly (stack overflow, so I'm not sure rentrant is that useful.

@bcardiff

This comment has been minimized.

Copy link
Member

bcardiff commented Dec 9, 2019

@ysbaddaden I think reentrant can be used for example in #8140 without ending in an infinite recursion. Lazy initializers are also a use case and the compiler checks there is no cyclic dependency IIRC.

@ysbaddaden ysbaddaden force-pushed the ysbaddaden:std/add-errorcheck-to-mutex-by-default branch from 7e8f194 to 116b1b9 Dec 9, 2019
@ysbaddaden

This comment has been minimized.

Copy link
Member Author

ysbaddaden commented Dec 9, 2019

Updated. I finally went for Protection (which is the actual term used in documentation, so it ain't that weird, and won't be used much publicly anyway) with :checked (default), :reentrant and :unchecked.

src/mutex.cr Outdated Show resolved Hide resolved
@RX14
RX14 approved these changes Dec 9, 2019
@RX14

This comment has been minimized.

Copy link
Member

RX14 commented Dec 9, 2019

Wonderful, thank you @ysbaddaden!

@ysbaddaden ysbaddaden force-pushed the ysbaddaden:std/add-errorcheck-to-mutex-by-default branch from 116b1b9 to 16c7aee Dec 9, 2019
@ysbaddaden

This comment has been minimized.

Copy link
Member Author

ysbaddaden commented Dec 9, 2019

Fixed typo.

@bcardiff bcardiff merged commit e725e1b into crystal-lang:master Dec 9, 2019
5 of 6 checks passed
5 of 6 checks passed
ci/circleci: test_preview_mt Your tests failed on CircleCI
Details
ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32_std Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ysbaddaden ysbaddaden deleted the ysbaddaden:std/add-errorcheck-to-mutex-by-default branch Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.