-
-
Notifications
You must be signed in to change notification settings - Fork 417
Makes core.sync.mutex.Mutex usable in modern code #1728
Makes core.sync.mutex.Mutex usable in modern code #1728
Conversation
core.sync.mutex.Mutex
usable in modern @safe nothrow @nogc
code
core.sync.mutex.Mutex
usable in modern @safe nothrow @nogc
codeb403473
to
fb291b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the comments, I think that this looks good. Long term, I would think that we'd want to force shared
on the whole API and deprecate the thread-local functions, but it's better to wait at least a release before doing that so that it's possible for folks to build their code for both the last release and master without getting a bunch of deprecation warnings that they can't fix.
throw new SyncError( "Unable to initialize mutex" ); | ||
scope(exit) pthread_mutexattr_destroy( &attr ); | ||
!pthread_mutexattr_init(&attr) || | ||
assert (0, "Unable to initialize mutex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core.internal.abort
would be closer to the previous functionality, since it would still print out in release mode. And given how incredibly confusing this failure would be if it were to ever occur without an error message, it would probably be better to use abort
in all the cases in this commit where you use assert(0)
. And it'll work with @nogc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -499,8 +499,11 @@ int pthread_key_delete(pthread_key_t); | |||
int pthread_mutex_destroy(pthread_mutex_t*); | |||
int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted; | |||
int pthread_mutex_lock(pthread_mutex_t*); | |||
int pthread_mutex_lock(shared(pthread_mutex_t)*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it make more sense to just leave the C functions as-is and cast away shared
when calling them? After all, shared
does not exist in C. I'm surprised that this even works. It's also pretty weird to have shared
on only some of them when they're all clearly in the same boat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately made that change because it bothered me when porting C and C++ code to D, which was directly using the platform specific APIs. Especially when porting a C project where there were absolutely zero abstractions and I needed to add casts to almost every function, since almost every function used pthread_mutex
directly and I wanted to use shared
instead of __gshared
.
I'm surprised that this even works
You shouldn't be, if you consider that strchr
works the same way in C++. In C all symbols (including functions) are mangled the same as their name (give or take an underscore at the beginning). That allows (sometimes correctly and sometimes not) to map multiple overloads from C++ or D to the same C function. (Note: some C++ compilers allow C function overloading but require vendor specific extensions similar to D's pragma (mangle)
.)
Also consider that in D you can overload extern (C)
functions on attributes like nothrow. Especially useful for function pointer parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also pretty weird to have shared on only some of them when they're all clearly in the same boat.
Not all of them can be called concurrently on the same object. For example pthread_mutex_destroy
is not thread-safe - it should be called only when the (conceptual) ref count of the mutex drops to 1. Also removing the non-shared
overloads would be breaking change.
Functions like pthread_rwlock_*lock
should also have shared overloads, but I prefer to leave this for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah shared is good here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: LDC does not allow multiple definitions that mangle to the same name, so this will probably come up again when updating druntime in LDC. Not sure whether that restriction should be lifted in LDC, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, if I remember correctly that was changed recently. The correct way to go about that in LDC, in my opinion, is to allow overloading if and only if all overloads have the same ABI as should be the case here.
To be pedantically correct, those functions should have been declared as taking shared
parameters in the first place, but leaving only the shared
versions would be a breaking change now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the approach to take, then pretty much every C/C++ function should have shared
parameters, because in reality, that's what they are. We just usually ignore that fact, because it usually doesn't matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like const
prevents accidental mutation, shared
guards against accidental use of non-thread-safe functions on shared data. Only functions that are proven to operate in a thread-safe way on shared data should be marked as shared.
In general, when we don't know whether a function is thread-safe we should not call it on shared data without sufficient synchronization and therefore we should not mark any of its parameters as shared
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is zero guarantee that operating on something that's shared
is thread-safe. If anything, it's the opposite. shared
means that the data is not thread-local. So, proper synchronization is required. To be perfectly correct, basically all extern(C) functions should be have shared
parameters and return types, because variables in C are not thread-local unless you jump through a bunch of hoops, using stuff that's not actually part of the language.
As it stands, we don't bother marking types on extern(C)
functions with shared
, and it's up to the programmer to understand what they're getting into and making sure that that they don't violate the thread-local guarantees that the D compiler provides for D code. If we then decide to start marking extern(C)
functions with shared
, the ones that most need to be marked are the ones that aren't thread-safe, not the ones that are.
Deciding to use shared
on an extern(C)
function because it's thread-safe, makes no sense at all. If it's thread-safe, then you can get way with passing thread-local data, whereas if it's not, then the data needs to be treated as shared
by the D code - whether that's by marking up the extern(C)
function with shared
or by having the D code just be aware of the semantics and and dealing with them accordingly.
So, if we want to start marking extern(C)
functions with shared
, then we should be marking the ones that clearly operate on data that is intended to be shared across threads as shared
, and whether the function is thread-safe is pretty much irrelevant.
@@ -192,7 +230,20 @@ class Mutex : | |||
* Returns: | |||
* true if the lock was acquired and false if not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to fix it so that tryLock
's documentation no longer says that it throws a SyncError
. And actually, that bit of documentation is wrong even without your changes, since it just calls a C function and doesn't throw anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
@@ -163,7 +167,7 @@ class Mutex : | |||
lock_nothrow(); | |||
} | |||
|
|||
// undocumented function for internal use | |||
/// ditto | |||
final void lock_nothrow(this Q)() nothrow @trusted @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you documenting these because the normal versions can't be made nothrow
and @nogc
without breaking code? You can overload on attributes, but that still breaks code since you get an error about overriding one of the functions shadowing the others. I can see someone overriding lock
and unlock
for debugging purposes, but it seems really dumb for the version of the function that you would ideally call to not be lock
or unlock
... Also, the _nothrow
function names don't actually follow the official naming convention, though lockNothrow
does seem a bit ugly. As such, I'd be inclined to find a better name for them, but I don't know what it would be. Something like lockMutex
would follow the naming convention and doesn't seem as ugly, but it seems redundant, and it doesn't make the difference between it and lock
obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since those methods (lock, unlock, tryLock) are virtual, we can't add attributes to them without breaking code. @MartinNowak attempted to enforce nothrow
back then when 2.067 was to be released, but that broke vibe.d
and had to be reverted. You can read more details about why vibe.d needed to throw from the locking primitives in this thread (in short - in order to implement interruptable tasks with proper resource cleanup).
I'm not 100% sure if we should document those functions, but that's what we should actually try to advertise as it is more friendly to code that tries to apply attributes where possible. I'm also not fan of the names, but considering that they're already public (albeit undocumented) I think we're kind of stuck with them. The only alternative names that I can think of are acquire and release, though tryAcquire sound bad to me.
Perhaps we can add lockExcept
/ unlockExcept
virtual functions and deprecate the throwing methods, but I prefer not to do that in this change.
b0fe0f1
to
2ceb5cc
Compare
Thanks for the review @jmdavis. I've added unittests and I've addressed all your points. |
Ping @andralex @WalterBright can you help me move forward with this PR? I need it continue working on adding more attributes in druntime and phobos. |
ping @jmdavis |
Nice work! |
@@ -18,11 +18,11 @@ module core.sync.mutex; | |||
|
|||
public import core.sync.exception; | |||
|
|||
version( Windows ) | |||
version (Windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noice
} | ||
|
||
/// ditto | ||
final void unlock_nothrow(this Q)() nothrow @trusted @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unlockNothrow
? (though truth be told the underscore convention is also used)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unlock_nothrow
was introduced in 2.067, so there's not much that I can do now.
@@ -499,8 +499,11 @@ int pthread_key_delete(pthread_key_t); | |||
int pthread_mutex_destroy(pthread_mutex_t*); | |||
int pthread_mutex_init(pthread_mutex_t*, pthread_mutexattr_t*) @trusted; | |||
int pthread_mutex_lock(pthread_mutex_t*); | |||
int pthread_mutex_lock(shared(pthread_mutex_t)*); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah shared is good here
OK let's move this forward once @jmdavis takes a second look |
Thanks for the review @andralex! |
Ping @jmdavis is there anything left for me to do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the one bit with pthread_mutexattr_destroy
should be fixed (which I missed on my previous review), and then it's ready to go. I'm not entirely enthused with the idea of marking some of the pthread
functions with shared
, but Andrei seems to be in favor, so it's okay, I guess.
|
||
if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) ) | ||
throw new SyncError( "Unable to initialize mutex" ); | ||
scope (exit) pthread_mutexattr_destroy(&attr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems inconsistent that the result of pthread_mutexattr_destroy
is not checked. It probably should be. I'm not sure that that's really blocker though, since it hasn't been being checked. It would just be an improvement that was missed with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll fix this later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed & rebased
Additionally add `shared` overloads to: + core.sys.windows.winbase: * EnterCriticalSection * LeaveCriticalSection * TryEnterCriticalSection + core.sys.posix.pthread: * pthread_mutex_lock * pthread_mutex_unlock * pthread_mutex_trylock Also add `tryLock_nothrow` version of `Mutex.tryLock`.
* Also make the error messages more descriptive
2ceb5cc
to
90b1db6
Compare
@jmdavis Merge pls? Even though I added three unittests codecov is complaining because the |
@@ -43,6 +42,9 @@ else | |||
|
|||
/** | |||
* This class represents a general purpose, recursive mutex. | |||
* | |||
* Implemented using pthread_mutex on Posix and CRITICAL_SECTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap pthread_mutex and CRITICAL_SECTION in backquotes
{ | ||
InitializeCriticalSection( &m_hndl ); | ||
InitializeCriticalSection(cast(CRITICAL_SECTION*)&m_hndl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after cast(...) cc @wilzbach
throw new SyncError( "Unable to initialize mutex" ); | ||
scope(exit) pthread_mutexattr_destroy( &attr ); | ||
!pthread_mutexattr_init(&attr) || | ||
abort("Error: pthread_mutexattr_init failed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No exclamation marks please, just use a period.
if( pthread_mutexattr_settype( &attr, PTHREAD_MUTEX_RECURSIVE ) ) | ||
throw new SyncError( "Unable to initialize mutex" ); | ||
scope (exit) !pthread_mutexattr_destroy(&attr) || | ||
abort("Error: pthread_mutexattr_destroy failed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
if( pthread_mutex_init( &m_hndl, &attr ) ) | ||
throw new SyncError( "Unable to initialize mutex" ); | ||
!pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE) || | ||
abort("Error: pthread_mutexattr_settype failed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
abort("Error: pthread_mutexattr_settype failed!"); | ||
|
||
!pthread_mutex_init(cast(pthread_mutex_t*)&m_hndl, &attr) || | ||
abort("Error: pthread_mutex_init failed!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
m_proxy.link = this; | ||
this.__monitor = &m_proxy; | ||
this.__monitor = cast(void*)&m_proxy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space
@@ -89,28 +106,43 @@ class Mutex : | |||
* In: | |||
* o must not already have a monitor. | |||
*/ | |||
this( Object o ) nothrow @trusted | |||
this(Object o) @trusted nothrow @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
o
is awkward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about obj
? Any better suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj
is fine - pretty much anything but o
, O
, I
, and l
:)
assert( !rc, "Unable to destroy mutex" ); | ||
import core.internal.abort : abort; | ||
!pthread_mutex_destroy(&m_hndl) || | ||
abort("Unable to destroy mutex"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period at end
That's incorrect. Most |
@andralex fixed & rebased. |
Yes and no. The C stuff isn't thread-local, and that means that technically, it's all Adjusting that approach so that we mark So, the most correct thing would be to marked the all as |
This is wrong, almost backwards actually. The lengthy explanation that follows does not help either :). Consider |
The issue is that unlike D code, the C code makes no guarantees about operating on thread-local data, because normally, everything in C is Basically, because So, moving to marking I still think that it would be safer though if the language treated So, I guess we disagree on what the default really should be with regards to |
No. Normally most in C is NOT shared, it's just that that information is not represented in the type system. Typical C programs have carefully-managed portions of data that is actually shared and specific narrow APIs for it. Don't confuse de jure with de facto.
Not if the D signature of the C binding is written correctly.
Most definitely
The onus is small - only for the handful of C functions that actually do perform synchronization. The situation doesn't warrant multiple walls of texts. |
That's completely backwards when it comes to the interface of a function. However ill-defined |
Auto-merge toggled on |
} | ||
|
||
/// ditto | ||
this() shared @trusted nothrow @nogc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ZombineDev
Unfortunately this broke sub-classes, see Issue 17130 – [Reg 2.074] ambiguous implicit super call when inheriting core.sync.mutex.Mutex. Would be nicer to fix this in dmd though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This indeed looks like a compiler bug. If the base class has multiple default constructors, the compiler should synthesize multiple default constructors in the derived class. I.e. the base class and the derived class should have equivalent default constructor overload sets. (Assuming no user-defined default constructors in the derived class.)
Do you think that the front-end can be fixed in a relatively short-term period? If not, probably the best course of action would be to revert this PR, though I prefer not to.
Hopefully the project tester can prevent such issues from coming up next time. I wonder why it didn't run for the whole month my PR was up for review.
Makes
core.sync.mutex.Mutex
usable in modern@safe nothrow @nogc
D code:Mutex.this
() /this(Object)
/~this()
nothrow
and@nogc
Mutex.this
,Mutex.lock
,Mutex.unlock
andMutex.tryLock
tryLock_nothrow
variant ofMutex.tryLock
- for use innothrow @nogc
code, similar toMutex.lock_nothrow
andMutex.unlock_nothrow
.Please review commit by commit, as it would be more clear why each change is made.