www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Is synchronized(...){...} doomed to never be nothrow/ nogc?

reply Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
Here is the situation, AIUI:

1. We allow users to provide their own Monitors, which hook into 
the synchronized(obj) statement.

2. These monitors' methods are unadorned (no nothrow/ nogc).

3. As a result, synchronized(obj) statements will not compile in 
nothrow/ nogc code, because we can't know that the Monitor 
implementation doesn't throw / use the GC.

4. As Matthias mentioned, fixing this was attempted before (by 
making the Monitor methods `nothrow`). However, this broke code 
(vibe.d specifically), as its implementation was actually not 
`nothrow`: https://issues.dlang.org/show_bug.cgi?id=11216

This is a problem e.g. in Druntime code, because it currently 
forces a lot of code that should be `nothrow` to not be annotated 
as such, even when the synchronization objects used use the 
standard D monitor implementation, which is actually `nothrow`.

So I guess the way forward here for the Druntime code is to 
abandon the synchronized() statement and use locks directly?
May 10 2016
next sibling parent reply Jack Stouffer <jack jackstouffer.com> writes:
On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev wrote:

 because we can't know that the Monitor implementation doesn't 
 throw / use the GC.


Why not? Surely this information is available at compile-time?
May 10 2016
parent Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Tuesday, 10 May 2016 at 21:01:38 UTC, Jack Stouffer wrote:

 On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev 
 wrote:

 because we can't know that the Monitor implementation doesn't 
 throw / use the GC.


 Why not? Surely this information is available at compile-time?


Well, not really, because it's done via an abstract interface. 
See Object.Monitor in object.d.
May 10 2016
prev sibling next sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/10/16 1:46 PM, Vladimir Panteleev wrote:

 Here is the situation, AIUI:

 1. We allow users to provide their own Monitors, which hook into the
 synchronized(obj) statement.

 2. These monitors' methods are unadorned (no nothrow/ nogc).

 3. As a result, synchronized(obj) statements will not compile in
 nothrow/ nogc code, because we can't know that the Monitor
 implementation doesn't throw / use the GC.

 4. As Matthias mentioned, fixing this was attempted before (by making
 the Monitor methods `nothrow`). However, this broke code (vibe.d
 specifically), as its implementation was actually not `nothrow`:
 https://issues.dlang.org/show_bug.cgi?id=11216

 This is a problem e.g. in Druntime code, because it currently forces a
 lot of code that should be `nothrow` to not be annotated as such, even
 when the synchronization objects used use the standard D monitor
 implementation, which is actually `nothrow`.

 So I guess the way forward here for the Druntime code is to abandon the
 synchronized() statement and use locks directly?


Does synchronized(someMutex) go through the same mechanism? Because it 
shouldn't, and then we can have the compiler properly interpret what the 
nothrow  nogc status is.

I think Monitor.lock/unlock should be nothrow/ nogc. I'm not familiar 
with the vibe.d code, but I'd believe that if you need those to throw or 
allocate, then you should have to go through another mechanism.

-Steve
May 10 2016
prev sibling next sibling parent Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Tuesday, May 10, 2016 17:46:17 Vladimir Panteleev via Digitalmars-d wrote:

 Here is the situation, AIUI:

 1. We allow users to provide their own Monitors, which hook into
 the synchronized(obj) statement.

 2. These monitors' methods are unadorned (no nothrow/ nogc).

 3. As a result, synchronized(obj) statements will not compile in
 nothrow/ nogc code, because we can't know that the Monitor
 implementation doesn't throw / use the GC.

 4. As Matthias mentioned, fixing this was attempted before (by
 making the Monitor methods `nothrow`). However, this broke code
 (vibe.d specifically), as its implementation was actually not
 `nothrow`: https://issues.dlang.org/show_bug.cgi?id=11216

 This is a problem e.g. in Druntime code, because it currently
 forces a lot of code that should be `nothrow` to not be annotated
 as such, even when the synchronization objects used use the
 standard D monitor implementation, which is actually `nothrow`.

 So I guess the way forward here for the Druntime code is to
 abandon the synchronized() statement and use locks directly?


Well, if polymorphism is involved at all, we're going to be screwed one way
or another with regards to attributes. Either we're going to be very
restrictive about what can be done in those functions by marking them with
attributes like  nogc and nothrow, or we're giong to be restrictive about
what code can use those functions by not marking them with attributes like
 nogc or nothrow. This is part of why inheritance and polymorphism tend to
be a very bad idea in D. OOP can obviously be very useful, but it comes at a
definite cost.

The only way to fix it so that we don't force one set of attributes or
another would be to stop using polymorphism, and make it so that all of the
code involved can be examined at compile time. So, either we figure out how
to do that, or we have to pick which attributes we want and deal with the
restrictions that come with them.

That being said, I have a hard time believing that it makes sense for these
primitives to involve exceptions - and even the GC likely shouldn't be
involved, particularly since these aren't the sorts of operations that would
normally allocate. So, I would think that aside from the breakage it would
cause, marking them as  nogc and nothrow would be perfectly acceptable. The
problem is that changing the attributes on a class or interface tends to
result in immediate code breakage, and I'm not sure that we really have a
fix for that.

- Jonathan M Davis
May 10 2016
prev sibling next sibling parent Mathias Lang <mathias.lang sociomantic.com> writes:
On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev wrote:

 Here is the situation, AIUI:

 1. We allow users to provide their own Monitors, which hook 
 into the synchronized(obj) statement.

 2. These monitors' methods are unadorned (no nothrow/ nogc).

 3. As a result, synchronized(obj) statements will not compile 
 in nothrow/ nogc code, because we can't know that the Monitor 
 implementation doesn't throw / use the GC.

 4. As Matthias mentioned, fixing this was attempted before (by 
 making the Monitor methods `nothrow`). However, this broke code 
 (vibe.d specifically), as its implementation was actually not 
 `nothrow`: https://issues.dlang.org/show_bug.cgi?id=11216

 This is a problem e.g. in Druntime code, because it currently 
 forces a lot of code that should be `nothrow` to not be 
 annotated as such, even when the synchronization objects used 
 use the standard D monitor implementation, which is actually 
 `nothrow`.

 So I guess the way forward here for the Druntime code is to 
 abandon the synchronized() statement and use locks directly?


As far as I remember, the issue was not the design, but that it 
required a design change on Vibe.d's side.

See 
https://github.com/rejectedsoftware/vibe.d/pull/972#issuecomment-75714736 where
Sonke mentions that "actually mutexes in vibe.d are never nothrow".
However, since some of the internals of Vibe.d are now supported 
upstream, the problem might disappear in a foreseeable future.
May 10 2016
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
Why does vibe.d throw?

Also, any throwing code can be converted to nothrow with:

    try
    {
	...throwing code...
    }
    catch (Exception e)
    {
	...
    }
May 10 2016
next sibling parent =?UTF-8?Q?S=c3=b6nke_Ludwig?= <sludwig outerproduct.org> writes:
Am 11.05.2016 um 03:08 schrieb Walter Bright:

 Why does vibe.d throw?

 Also, any throwing code can be converted to nothrow with:

     try
     {
      ...throwing code...
     }
     catch (Exception e)
     {
      ...
     }


There are two kinds of fiber-aware mutexes in vibe.d, one that doesn't 
throw (created when this issue first came up) and the original one that 
can be interrupted from a different task using `Task.interrupt()`. In 
case of interruption it will throw an InterruptException instead of 
continuing to wait for the lock.
May 10 2016
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2016-05-11 03:08, Walter Bright wrote:

 Why does vibe.d throw?

 Also, any throwing code can be converted to nothrow with:

     try
     {
      ...throwing code...
     }
     catch (Exception e)
     {
      ...
     }


There's code in DWT and Tango as well that throws a SyncException 
exception when failing to acquire a lock.

The code in DWT is basically copy pasted from Tango. Since druntime is 
derived from Tango I guess we can update the code in DWT and Tango to do 
what druntime does now, which is to throw an error instead.

It's still a breaking change.

Although I see now that there is a problem with updating the code in 
DWT. In some cases it catches SyncException with some logic based on 
that. In one case the try-catch is wrapped in a loop and will continue 
to try to acquire the look [1]. In another case it catches SyncException 
and triggers an interrupt [2], which I see now is not implemented [3].

Please advise how these two cases should be implemented when a lock will 
throw an error instead of exception (since errors should not be caught).

Hmm, maybe I can extend the API. Monitor is not used directly in any of 
the cases.

[1] 
https://github.com/d-widget-toolkit/org.eclipse.swt.gtk.linux.x86/blob/master/src/org/eclipse/swt/internal/Lock.d#L55

[2] 
https://github.com/d-widget-toolkit/org.eclipse.swt.gtk.linux.x86/blob/master/src/org/eclipse/swt/widgets/Synchronizer.d#L192-L198

[3] 
https://github.com/d-widget-toolkit/org.eclipse.swt.gtk.linux.x86/blob/master/src/org/eclipse/swt/internal/Compatibility.d#L402-L405

-- 
/Jacob Carlborg
May 11 2016
next sibling parent reply ZombineDev <petar.p.kirov gmail.com> writes:
On Wednesday, 11 May 2016 at 07:01:43 UTC, Jacob Carlborg wrote:

 On 2016-05-11 03:08, Walter Bright wrote:

     [...]


 There's code in DWT and Tango as well that throws a 
 SyncException exception when failing to acquire a lock.

 [...]


These cases all look like they want to use tryLock instead
May 11 2016
parent Jacob Carlborg <doob me.com> writes:
On 2016-05-11 09:22, ZombineDev wrote:


 These cases all look like they want to use tryLock instead



Ah, thanks. Although I see now that the code is not using Monitor.lock, 
it uses Condition.wait. Condition in this case is not the one in 
druntime, so I might be good.

BTW, shouldn't there be a tryWait as well?

-- 
/Jacob Carlborg
May 11 2016
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 5/11/2016 12:01 AM, Jacob Carlborg wrote:

 There's code in DWT and Tango as well that throws a SyncException exception
when
 failing to acquire a lock.

 The code in DWT is basically copy pasted from Tango. Since druntime is derived
 from Tango I guess we can update the code in DWT and Tango to do what druntime
 does now, which is to throw an error instead.

 It's still a breaking change.

 Although I see now that there is a problem with updating the code in DWT. In
 some cases it catches SyncException with some logic based on that. In one case
 the try-catch is wrapped in a loop and will continue to try to acquire the look
 [1].


Perhaps instead of throwing an exception it can try to re-acquire at the lower 
level.


 In another case it catches SyncException and triggers an interrupt [2],
 which I see now is not implemented [3].


I suppose the code has to decide if it is a fatal error or not.
May 11 2016
parent Jacob Carlborg <doob me.com> writes:
On 2016-05-11 13:10, Walter Bright wrote:


 Perhaps instead of throwing an exception it can try to re-acquire at the
 lower level.


There is basically no lower level, except the low level abstraction 
which wraps the Posix or Windows call. But it might be that the code is 
not affected. It's actually calling Condition.wait and not Monitor.lock 
in the cases where it catches the exception.

-- 
/Jacob Carlborg
May 12 2016
prev sibling parent reply Dicebot <public dicebot.lv> writes:
On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev wrote:

 So I guess the way forward here for the Druntime code is to 
 abandon the synchronized() statement and use locks directly?


I believe this is the way. Synchronized statements don't add any 
crucial value compared to plain locks. At the same time 
forbidding throwing from even more runtime overrides would be 
both annoying and unnecessary restrictive.
May 11 2016
next sibling parent Guillaume Piolat <first.last gmail.com> writes:
On Wednesday, 11 May 2016 at 07:05:07 UTC, Dicebot wrote:

 On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev 
 wrote:

 So I guess the way forward here for the Druntime code is to 
 abandon the synchronized() statement and use locks directly?


 I believe this is the way. Synchronized statements don't add 
 any crucial value compared to plain locks. At the same time 
 forbidding throwing from even more runtime overrides would be 
 both annoying and unnecessary restrictive.


+1

For the sake of  nogc, I can't use druntime locks or synchronized 
anyway.
May 11 2016
prev sibling next sibling parent Walter Bright <newshound2 digitalmars.com> writes:
On 5/11/2016 12:05 AM, Dicebot wrote:

 I believe this is the way. Synchronized statements don't add any crucial value
 compared to plain locks. At the same time forbidding throwing from even more
 runtime overrides would be both annoying and unnecessary restrictive.


Also, the synchronized statement preceded the introduction of RAII, so perhaps 
it is obsolete.
May 11 2016
prev sibling parent reply Dicebot <public dicebot.lv> writes:
On Wednesday, 11 May 2016 at 07:05:07 UTC, Dicebot wrote:

 On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev 
 wrote:

 So I guess the way forward here for the Druntime code is to 
 abandon the synchronized() statement and use locks directly?


 I believe this is the way. Synchronized statements don't add 
 any crucial value compared to plain locks. At the same time 
 forbidding throwing from even more runtime overrides would be 
 both annoying and unnecessary restrictive.


It is probably also worth re-iterating on my long standing 
position that adding more `nothrow` in fundamental facilities is 
a false goal and almost always does more harm than good. 
Exceptions are main (and pretty much only) error handling 
facility in D. By adding `nothrow` base runtime class methods you 
make impossible for D developers to use standard language 
facilities and force uncalled hacks to workaround it.

`nothrow` is much more intrusive than ` safe` or `pure` in that 
sense and should not be applied blindly to everything simply 
because it is possible.
May 11 2016
next sibling parent Vladimir Panteleev <thecybershadow.lists gmail.com> writes:
On Wednesday, 11 May 2016 at 11:16:41 UTC, Dicebot wrote:

 It is probably also worth re-iterating on my long standing 
 position that adding more `nothrow` in fundamental facilities 
 is a false goal and almost always does more harm than good. 
 Exceptions are main (and pretty much only) error handling 
 facility in D. By adding `nothrow` base runtime class methods 
 you make impossible for D developers to use standard language 
 facilities and force uncalled hacks to workaround it.

 `nothrow` is much more intrusive than ` safe` or `pure` in that 
 sense and should not be applied blindly to everything simply 
 because it is possible.


Sometimes you really, actually must ensure that things aren't 
going to throw, such as in a signal handler, unmanaged thread, 
between fork and exec, bare-metal, ...

Your point is orthogonal to the goal of adding nothrow/ nogc 
without breaking user code, and I don't disagree with it.
May 11 2016
prev sibling parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Wednesday, May 11, 2016 11:16:41 Dicebot via Digitalmars-d wrote:

 On Wednesday, 11 May 2016 at 07:05:07 UTC, Dicebot wrote:

 On Tuesday, 10 May 2016 at 17:46:17 UTC, Vladimir Panteleev

 wrote:

 So I guess the way forward here for the Druntime code is to
 abandon the synchronized() statement and use locks directly?


 I believe this is the way. Synchronized statements don't add
 any crucial value compared to plain locks. At the same time
 forbidding throwing from even more runtime overrides would be
 both annoying and unnecessary restrictive.


 It is probably also worth re-iterating on my long standing
 position that adding more `nothrow` in fundamental facilities is
 a false goal and almost always does more harm than good.
 Exceptions are main (and pretty much only) error handling
 facility in D. By adding `nothrow` base runtime class methods you
 make impossible for D developers to use standard language
 facilities and force uncalled hacks to workaround it.

 `nothrow` is much more intrusive than ` safe` or `pure` in that
 sense and should not be applied blindly to everything simply
 because it is possible.


Regardless of the desirability of marking stuff with nothrow, one _huge_
difference between nothrow and pure or  nogc is that it's trivial to use a
function that throws inside of nothrow code by wrapping it in a try-catch
block.  safe is in a similar boat thanks to  trusted, but pure and  nogc can
only be gotten around via some nasty casts.

That being said, I think that anything that really shouldn't ever throw
should be marked nothrow, but we need to be sure that it really doesn't ever
make sense for it to throw before we mark it as nothrow. If it does make
sense for it to throw under any circumstances, then we can't mark it as
nothrow, and then anyone who wants nothrow is going to have to use try-catch
blocks to use it in nothrow code.

- Jonathan M Davis
May 11 2016
parent reply Dicebot <public dicebot.lv> writes:
On Wednesday, 11 May 2016 at 14:12:47 UTC, Jonathan M Davis wrote:

 Regardless of the desirability of marking stuff with nothrow, 
 one _huge_ difference between nothrow and pure or  nogc is that 
 it's trivial to use a function that throws inside of nothrow 
 code by wrapping it in a try-catch block.  safe is in a similar 
 boat thanks to  trusted, but pure and  nogc can only be gotten 
 around via some nasty casts.


It doesn't apply if throwing is desired as part of API, which is 
exactly the case for monitor and issue I had before with adding 
nothrow to some Fiber methods.
May 11 2016
parent reply Jonathan M Davis via Digitalmars-d <digitalmars-d puremagic.com> writes:
On Wednesday, May 11, 2016 14:22:39 Dicebot via Digitalmars-d wrote:

 On Wednesday, 11 May 2016 at 14:12:47 UTC, Jonathan M Davis wrote:

 Regardless of the desirability of marking stuff with nothrow,
 one _huge_ difference between nothrow and pure or  nogc is that
 it's trivial to use a function that throws inside of nothrow
 code by wrapping it in a try-catch block.  safe is in a similar
 boat thanks to  trusted, but pure and  nogc can only be gotten
 around via some nasty casts.


 It doesn't apply if throwing is desired as part of API, which is
 exactly the case for monitor and issue I had before with adding
 nothrow to some Fiber methods.


I wasn't disagreeing with that. If there are valid uses cases for a monitor
throwing, then its functions should not be nothrow. My point is that if
those functions aren't marked with nothrow, it's trivial to use them in
nothrow code (even if it's a bit ugly) by wrapping them in try-catch blocks,
whereas attributes like pure or  nogc are not so easy to get around. pure
wouldn't apply in this case, but  nogc might. However, until the issues with
using exceptions without the GC are resolved, as long as monitor's functions
can be nothrow, it makes no sense to mark them with  nogc.

So, the lack of nothrow is far from a blocker for anyone wanting nothrow
code. It's  nogc that's more of an issue, but we're going to need language
improvements before we can consider using  nogc in this case.

- Jonathan M Davis
May 11 2016
parent Dicebot <public dicebot.lv> writes:
On Wednesday, 11 May 2016 at 14:54:07 UTC, Jonathan M Davis wrote:

 On Wednesday, May 11, 2016 14:22:39 Dicebot via Digitalmars-d 
 wrote:

 On Wednesday, 11 May 2016 at 14:12:47 UTC, Jonathan M Davis 
 wrote:

 Regardless of the desirability of marking stuff with 
 nothrow, one _huge_ difference between nothrow and pure or 
  nogc is that it's trivial to use a function that throws 
 inside of nothrow code by wrapping it in a try-catch block. 
  safe is in a similar boat thanks to  trusted, but pure and 
  nogc can only be gotten around via some nasty casts.


 It doesn't apply if throwing is desired as part of API, which 
 is exactly the case for monitor and issue I had before with 
 adding nothrow to some Fiber methods.


 I wasn't disagreeing with that. If there are valid uses cases 
 for a monitor throwing, then its functions should not be 
 nothrow. My point is that if those functions aren't marked with 
 nothrow, it's trivial to use them in nothrow code (even if it's 
 a bit ugly) by wrapping them in try-catch blocks, whereas 
 attributes like pure or  nogc are not so easy to get around. 
 pure wouldn't apply in this case, but  nogc might. However, 
 until the issues with using exceptions without the GC are 
 resolved, as long as monitor's functions can be nothrow, it 
 makes no sense to mark them with  nogc.

 So, the lack of nothrow is far from a blocker for anyone 
 wanting nothrow code. It's  nogc that's more of an issue, but 
 we're going to need language improvements before we can 
 consider using  nogc in this case.

 - Jonathan M Davis


Sorry, I got your point exactly backwards :)
May 11 2016