New `synchronized`

[Charles]

In general, .NET enables an entire method to be synchronized/locked with the MethodImpl attribute:

use System.Runtime.CompilerServices

class X

    def foo has MethodImpl(MethodImplOptions.Synchronized)
        # do some stuff

Not very attractive, is it? I have added some syntactic sugar for this:

class X

    def foo is synchronized
        # do some stuff

The block-level equivalent is the lock statement which makes me wonder if we should say "is locked" instead for consistency:

class X

    def foo is locked
        # do some stuff

Also, in C# discussions on using the attribute vs. the lock() statement around the entire method body, there seems to be some confusion as to whether or not using the attribute is acceptable. If we find out it's not, the Cobra implementation for "is synchronized" or "is locked" would be changed to generate the lock statement around the method body.

Or maybe this feature is a bad idea because "the body of a lock should do as little as possible" and it may encourage the opposite.

Opinions?

P.S. You can browse tests cases here if you're interested to see them.

[hopscc]

The syntactic sugar is good (better than the attribute) but it should be named the same as the block level equivalent.
i.e use locked

Yes locked areas should be as small as possible and this may be abused but sometimes it is what you need....
Adding locking calls at whatever level is not necessarily sufficient to avoid all deadlocks...

Is there a C# equivalent at a class level ?
(locking/synchronising on a class makes all accesses (fields/methods/events) through that class instances also locked )
If not should cobra also provide one ?
( cf java synchronised classes)

[nerdzero]

Cool stuff. I like it.

I am indifferent regarding 'synchronized' vs. 'locked' but is there any concern that the locking object would be 'this'? This is supposedly "bad" because you can't control others locking on your object: http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx

from Remarks section of that link:

In general, avoid locking on a public type, or instances beyond your code's control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

Code: Select all

lock (this)

is a problem if the instance can be accessed publicly.

Code: Select all

lock (typeof (MyType))

is a problem if MyType is publicly accessible.

Code: Select all

lock("myLock")

is a problem because any other code in the process using the same string, will share the same lock.
Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.

Personally, I think locking on 'this' is fine but just wanted to bring up that most of the experts say it invites deadlocks. Maybe the sugar we really want is turning this code:

class Foo

    var __thisLock = Object()

    def bar
        lock __thisLock
            ...

into this code:

class Foo

    def bar is locked
        ...

[Charles]

Re: locked classes, seems like it would be really rare.

Re: lock(this), there was a ton of discussion on lock(this) and then more discussion about improvement in .NET 4 or 4.5 and I got the impression that the attribute had been improved to do better. But the comments were not as clear as I would have liked... so I was not sure.

I'm also interested in the idea of allowing a `lock` statement without an argument in which case the private, shared Object() instance would be auto-generated for you. This would give a least resistance path that avoided the tempting "lock this".

[nerdzero]

I'm also interested in the idea of allowing a `lock` statement without an argument in which case the private, shared Object() instance would be auto-generated for you. This would give a least resistance path that avoided the tempting "lock this".

Yeah, that would be nice to have.

[todd.a]

I think it's fine to have the lock applied to the entire method. I'm not sure if it would encourage further abuse but as it currently stands someone can abuse it either way by locking a huge block. What about nested locks? Would it be possible to use the lock statement within such a method? It's a slippery slope (recipe for a deadlock) once you start doing that but sometimes it's what you need to do.

I like 'is synchronized' better, 'is locked' sort of gives me a different connotation.

Addtion: I'm referring to different lock objects. There's also the scenario where the locked method can be called from inside a different lock context. I'm not worried about different locked methods calling each other on the same thread.

[snakeman]

As far as i understand the concept, i'd find the following a bit more natural:

class X
    def foo is locking
        # do something

I'm new to the cobra and .net world. So please don't weight my opinion too much.

[Charles]

At http://msdn.microsoft.com/en-us/library ... tions.aspx it says for "Synchronized":

The method can be executed by only one thread at a time. Static methods lock on the type, whereas instance methods lock on the instance. Only one thread can execute in any of the instance functions, and only one thread can execute in any of a class's static functions.

And that's for .NET 4.5, the latest. And both lock(this) and lock(typeof(Foo)) are still recommended against in various places. So my current thoughts are to:

-- Change the name from "...is synchronized" to "...is locked".

-- Change the implementation to use a private (or protected) lock object which will be an instance var or shared var based on the method using it. The var is shared between methods of the same level (instance or shared). If you need something different, use explicit locks.

-- Permit the "lock" statement to be used without an argument, in which case the aforementioned private lock object will be used.

There is the question of inheritance. If class B inherits class A and they both have instance methods that are locked, does B reuse the lock object from A, or declare its own? I think it reuses. If so, that could introduce complications regarding libraries.

Also, in both C# and Java, locking a string is a source of errors because any string can be interned and therefore shared (search the web for "interned strings" if you need to). Cobra could issue a warning or error for this case. AFAICT it's never needed since there are other objects you could lock on.

Finally, I'll probably just kill "is synchronized" for now, as I don't want to hold up a release for straightening this all out.

Thoughts?

[nerdzero]

There is the question of inheritance. If class B inherits class A and they both have instance methods that are locked, does B reuse the lock object from A, or declare its own? I think it reuses. If so, that could introduce complications regarding libraries.

I think it should be reused. Having multiple locks without a well disciplined locking order increases the likelihood of deadlocks. There's always the "nonvirtual" keyword to prevent inheritance or overriding; and if you do need multiple locking objects, they can be declared explicitly.

Also, in both C# and Java, locking a string is a source of errors because any string can be interned and therefore shared (search the web for "interned strings" if you need to). Cobra could issue a warning or error for this case. AFAICT it's never needed since there are other objects you could lock on.

An error makes sense here. I've never heard of cases where locking on a String was required.