Writing multithreaded class in .net ( C#)

[KK]

Dear All
My requirement is to write class that is thread safe.( All public methods &
static methods must be thread safe ).
I found two options to implement this
1.using MethodImplOptions.Synchronized attribute
2.Using Lock statement

Class using method 1:
public class X
{

[MethodImpl(MethodImplOptions.Synchronized)]

public void X1()

{
}
[MethodImpl(MethodImplOptions.Synchronized)]

public void X2()
{

}
}

Class using method 2:
public class Y
{
public void Y1()
{
lock(this)
{
}
}
public void Y2()
{
lock(this)
{
}

}
}

Can somebody explain me what are the advantages & disadvantages with
these two approaches?

Thanks in advance.

Best Regards
Krishna

[Jon Skeet [C# MVP]]

KK wrote:

My requirement is to write class that is thread safe.( All public methods &
static methods must be thread safe ).
I found two options to implement this
1.using MethodImplOptions.Synchronized attribute
2.Using Lock statement

Using MethodImplOptions.Synchronized is equivalent to wrapping the body
of the method in:

lock (this)

or

lock (typeof(TheTypeInvolved))

I wouldn't recommend it, and I wouldn't recommend using "lock this"
anyway.

See http://www.pobox.com/~skeet/csharp/t...ckchoice.shtml for
reasons and recommendations.

Jon

[Marc Gravell]

Good article, and the one linked underneath it about an alternative (using)
syntax for Monitor; can I ask a quick question on the latter?

Firstly - the example code includes:

<quote>
// ... or if you want to be able to wait/pulse the monitor
using (LockToken token = syncLock.Lock(10000))
{
Monitor.Wait (token.Monitor);
// or
Monitor.Pulse (token.Monitor);
}
</quote>

First - looking at the code, I think this is a typo of syncLock.Monitor, but
my real question is: why make the monitor publicly visible at all? Why not
just have a SyncLock.Wait, SyncLock.Pulse and SyncLock.PulseAll (or better
still: LockToken.{Wait|Pulse|PulseAll}, since we really should own the lock
before calling these methods) that forwards via System.Threading.Monitor -
this would avoid callers having to mix implementations
(System.Threading.Monitor for some things, SyncLock / LockToken for others).
Also the SyncLock.monitor (field) could possibly be readonly (maybe it's
just my habit, but I tend to make my lock-objects readonly so I don't
accidentally break them by switching values - I'm sometimes clumsy that
way).

In case tone doesn't carry in an e-mail, I don't mean for this to sound
negative in any way - I think it's a very neat implementation : just some
thoughts and questions.

"Jon Skeet [C# MVP]" <sk***@pobox.com> wrote in message
news:11**********************@u72g2000cwu.googlegr oups.com...

KK wrote:

My requirement is to write class that is thread safe.( All public methods
&
static methods must be thread safe ).
I found two options to implement this
1.using MethodImplOptions.Synchronized attribute
2.Using Lock statement

Using MethodImplOptions.Synchronized is equivalent to wrapping the body
of the method in:

lock (this)

or

lock (typeof(TheTypeInvolved))

I wouldn't recommend it, and I wouldn't recommend using "lock this"
anyway.

See http://www.pobox.com/~skeet/csharp/t...ckchoice.shtml for
reasons and recommendations.

Jon

[Jon Skeet [C# MVP]]

Marc Gravell wrote:

Good article, and the one linked underneath it about an alternative (using)
syntax for Monitor; can I ask a quick question on the latter?

Firstly - the example code includes:

<quote>
// ... or if you want to be able to wait/pulse the monitor
using (LockToken token = syncLock.Lock(10000))
{
Monitor.Wait (token.Monitor);
// or
Monitor.Pulse (token.Monitor);
}
</quote>

First - looking at the code, I think this is a typo of syncLock.Monitor
Yup - things unfortunately changed in the API and I didn't catch them
all in the docs.
In fact, those docs don't do it justice now. If you look at
http://www.pobox.com/~skeet/csharp/m...e/locking.html
there's much more information, including deadlock detecting locks.
but my real question is: why make the monitor publicly visible at all? Why not
just have a SyncLock.Wait, SyncLock.Pulse and SyncLock.PulseAll (or better
still: LockToken.{Wait|Pulse|PulseAll}, since we really should own the lock
before calling these methods) that forwards via System.Threading.Monitor -
this would avoid callers having to mix implementations
(System.Threading.Monitor for some things, SyncLock / LockToken for others).
Also the SyncLock.monitor (field) could possibly be readonly (maybe it's
just my habit, but I tend to make my lock-objects readonly so I don't
accidentally break them by switching values - I'm sometimes clumsy that
way).
The problem is that I can't guarantee to expose *all* the methods of
Monitor, because in some future version it might gain some methods
which I won't know about, and can't add in without breaking its
compatibility with current versions. There's a bit more about this on
the page referred to above. As I say there, I'm willing to
reconsider...
In case tone doesn't carry in an e-mail, I don't mean for this to sound
negative in any way - I think it's a very neat implementation : just some
thoughts and questions.

No, that's fine :)

Jon

[Marc Gravell]

That fills in some blanks - thank you.

Marc

"Jon Skeet [C# MVP]" <sk***@pobox.com> wrote in message
news:11**********************@j33g2000cwa.googlegr oups.com...

Marc Gravell wrote:

Good article, and the one linked underneath it about an alternative
(using)
syntax for Monitor; can I ask a quick question on the latter?

Firstly - the example code includes:

<quote>
// ... or if you want to be able to wait/pulse the monitor
using (LockToken token = syncLock.Lock(10000))
{
Monitor.Wait (token.Monitor);
// or
Monitor.Pulse (token.Monitor);
}
</quote>

First - looking at the code, I think this is a typo of syncLock.Monitor

Yup - things unfortunately changed in the API and I didn't catch them
all in the docs.
In fact, those docs don't do it justice now. If you look at
http://www.pobox.com/~skeet/csharp/m...e/locking.html
there's much more information, including deadlock detecting locks.

but my real question is: why make the monitor publicly visible at all?
Why not
just have a SyncLock.Wait, SyncLock.Pulse and SyncLock.PulseAll (or
better
still: LockToken.{Wait|Pulse|PulseAll}, since we really should own the
lock
before calling these methods) that forwards via
System.Threading.Monitor -
this would avoid callers having to mix implementations
(System.Threading.Monitor for some things, SyncLock / LockToken for
others).
Also the SyncLock.monitor (field) could possibly be readonly (maybe it's
just my habit, but I tend to make my lock-objects readonly so I don't
accidentally break them by switching values - I'm sometimes clumsy that
way).

The problem is that I can't guarantee to expose *all* the methods of
Monitor, because in some future version it might gain some methods
which I won't know about, and can't add in without breaking its
compatibility with current versions. There's a bit more about this on
the page referred to above. As I say there, I'm willing to
reconsider...

In case tone doesn't carry in an e-mail, I don't mean for this to sound
negative in any way - I think it's a very neat implementation : just some
thoughts and questions.

No, that's fine :)

Jon

[Marc Gravell]

Hi again; I'm a convert... kind-of...

I toyed with this on the train, and agree that not only is it a nicer
wrapper around lock, but it offers significant development gain (such as the
named locks, which are invaluable). With some changes (see below) I was able
to use this to successfully identify a deadlock issue (in a highly-threaded
system) that has been bugging me for a while:

Some more feedback, however, on the knife-edge call about Pulse, Wait etc;
to assist my debugging, I pushed these methods (as private internal virtual)
onto SyncLock, and forwarded them (as public) from LockToken; this allowed
me to put in some #if DEBUG regions around Lock, Unlock, Wait and Pulse
which keep track of the thread currently holding the lock (in a Thread field
that is likewise #if DEBUG). By doing this, when a timeout occurs I can
include verbose information about exactly which 2 threads are competing. A
bit similar to some of OrderedLock, but my code wouldn't lend itself to an
OrderedLock implementation (without significant rework), where-as with
SyncLock it is an issue. As a side-benefit, it would also allow java-like
"do I hold the lock, else who" calls, but I'm not sure it is worth the
overhead except in DEBUG mode.

Re your thoughts about extensions to Monitor; while I see your point, if
additional functionality is added to Monitor that can't be added to
SyncLock, then a: I'd be suprised (as it would be an equal pain to use in
*any* code - perhaps more as not encapsulated), and b: as a class to do a
specific job, it works very well - so if something *couldn't* be
retro-fitted to SyncLock, then it is probably so esoteric that it wouldn't
want to be.

So, erm, yes; my /personal/ opinion is that this fits a lot better with
Pulse etc on the LockToken, as not only does it allow for a complete locking
solution (encapsulated in one consistent place), but it allows for positive
(debugging) benefit along the way.

(If you want any of my hacks, let me know and I'll e-mail them - but they're
pretty simple to be honest).

Cheers again for (indirectly, but crucially) helping me fix my [dead]locking
woes ;-p

Marc
"Jon Skeet [C# MVP]" <sk***@pobox.com> wrote in message
news:11**********************@j33g2000cwa.googlegr oups.com...

Marc Gravell wrote:

Good article, and the one linked underneath it about an alternative
(using)
syntax for Monitor; can I ask a quick question on the latter?

Firstly - the example code includes:

<quote>
// ... or if you want to be able to wait/pulse the monitor
using (LockToken token = syncLock.Lock(10000))
{
Monitor.Wait (token.Monitor);
// or
Monitor.Pulse (token.Monitor);
}
</quote>

First - looking at the code, I think this is a typo of syncLock.Monitor

Yup - things unfortunately changed in the API and I didn't catch them
all in the docs.
In fact, those docs don't do it justice now. If you look at
http://www.pobox.com/~skeet/csharp/m...e/locking.html
there's much more information, including deadlock detecting locks.

but my real question is: why make the monitor publicly visible at all?
Why not
just have a SyncLock.Wait, SyncLock.Pulse and SyncLock.PulseAll (or
better
still: LockToken.{Wait|Pulse|PulseAll}, since we really should own the
lock
before calling these methods) that forwards via
System.Threading.Monitor -
this would avoid callers having to mix implementations
(System.Threading.Monitor for some things, SyncLock / LockToken for
others).
Also the SyncLock.monitor (field) could possibly be readonly (maybe it's
just my habit, but I tend to make my lock-objects readonly so I don't
accidentally break them by switching values - I'm sometimes clumsy that
way).

The problem is that I can't guarantee to expose *all* the methods of
Monitor, because in some future version it might gain some methods
which I won't know about, and can't add in without breaking its
compatibility with current versions. There's a bit more about this on
the page referred to above. As I say there, I'm willing to
reconsider...

In case tone doesn't carry in an e-mail, I don't mean for this to sound
negative in any way - I think it's a very neat implementation : just some
thoughts and questions.

No, that's fine :)

Jon

[Jon Skeet [C# MVP]]

Marc Gravell <ma**@nonesuch.com> wrote:

Hi again; I'm a convert... kind-of...

I toyed with this on the train, and agree that not only is it a nicer
wrapper around lock, but it offers significant development gain (such as the
named locks, which are invaluable). With some changes (see below) I was able
to use this to successfully identify a deadlock issue (in a highly-threaded
system) that has been bugging me for a while:
Excellent. It's nice to see it being used "in the wild" as it were.
Some more feedback, however, on the knife-edge call about Pulse, Wait etc;
to assist my debugging, I pushed these methods (as private internal virtual)
onto SyncLock, and forwarded them (as public) from LockToken; this allowed
me to put in some #if DEBUG regions around Lock, Unlock, Wait and Pulse
which keep track of the thread currently holding the lock (in a Thread field
that is likewise #if DEBUG). By doing this, when a timeout occurs I can
include verbose information about exactly which 2 threads are competing. A
bit similar to some of OrderedLock, but my code wouldn't lend itself to an
OrderedLock implementation (without significant rework), where-as with
SyncLock it is an issue. As a side-benefit, it would also allow java-like
"do I hold the lock, else who" calls, but I'm not sure it is worth the
overhead except in DEBUG mode.
Right. Unfortunately, the way that debug/release builds work, you'd
have to have two different versions of the library available (or build
it alongside your project). It's not a great situation :(
Re your thoughts about extensions to Monitor; while I see your point, if
additional functionality is added to Monitor that can't be added to
SyncLock, then a: I'd be suprised (as it would be an equal pain to use in
*any* code - perhaps more as not encapsulated), and b: as a class to do a
specific job, it works very well - so if something *couldn't* be
retro-fitted to SyncLock, then it is probably so esoteric that it wouldn't
want to be.
The problem is a bit more subtle than that though. Suppose .NET 2.1
adds a new method in Monitor:

1) Until I update the source, no-one can use that new method in
conjunction with SyncLock
2) I can't provide a version which works against 1.1/2.0 *and* 2.1,
which would still have all the appropriate methods. Currently I'm just
building against 1.1, and anyone can use the library for 1.1 or 2.0.
I'd rather not get into the business of building multiple versions.

However, all of this rests on Monitor adding methods. I'm tempted to
take the risk...

So, erm, yes; my /personal/ opinion is that this fits a lot better with
Pulse etc on the LockToken, as not only does it allow for a complete locking
solution (encapsulated in one consistent place), but it allows for positive
(debugging) benefit along the way.

(If you want any of my hacks, let me know and I'll e-mail them - but they're
pretty simple to be honest).
If you could mail me the code, it would be handy - I generally find it
easier to understand actual code than descriptions of code :)
Cheers again for (indirectly, but crucially) helping me fix my [dead]locking
woes ;-p

My pleasure - and let me know if you have any more feedback...

--
Jon Skeet - <sk***@pobox.com>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too