Is this thread safe?

[Dan Bass]

I know that XslTransform's Transform is thread safe according to the MSDN,
and that Load is not. I've therefore applied this simply Mutex to it and
would just like to confirm this is okay.


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
mut.WaitOne();

xslt = new XslTransform();
xslt.Load( configFile );

mut.ReleaseMutex();
}
else
{
throw ( new ApplicationException ( "Configuration error,
XSL file does not exist. " + configFile ) );
}
}

//
// rest of the function code here

}

 

[Dan Bass]

I know that is wrong! Just realised the problem it could give me...
What about this?


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{

xslt = new XslTransform();
xslt.Load( configFile );

}
else
{
mut.ReleaseMutex();
throw ( new ApplicationException ( "Configuration error,
XSL file does not exist. " + configFile ) );
}
}

mut.ReleaseMutex();

//
// rest of the function code here

}

 

[Nicholas Paldino [.NET/C# MVP]]

Dan,

I wouldn't use a Mutex. I would just make the method synchronized, like
so:

[MethodImpl(MethodImplOptions.Synchronized)]
public override string MapMessage ( string messageSource )
{
// create and load up a new transform if required
mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
}
else
{
throw ( new ApplicationException ( "Configuration error, XSL file
does not exist. " + configFile));
}

// rest of the function code here
}

Hope this helps.

 

[Mr. Mountain]

Hi Dan,
I wouldn't use a Mutex. I would recommend using Monitor. There area couple
options. The most common is to use the C# lock statement, but this has some
drawbacks. Most notably, it is equivalent to a Monitor.TryEnter with an
infinite timeout. Therefore, a lot of the smart guys recommend using
Monitor.TryEnter with a finite timeout instead.

You may want to check out the articles and utility library by Jon Skeet at:
http://www.yoda.arachsys.com/csharp/threads/alternative.shtml

and a related blog article by Ian G at:
http://www.interact-sw.co.uk/iangblog/2004/04/26/yetmoretimedlocking
http://www.interact-sw.co.uk/iangblog/2004/03/23/locking

Also, if you code this on your own using Monitor.TryEnter, make sure you
start a try block immediately after you get the monitor and then exit the
monitor in a finally block.

The links above will give you full details on this and even some ready to
use code.

Regards,
Mountain

 

[Mr. Mountain]

Hi Nicholas,
If I understand correctly, using the MethodImpl attribute is the same as a
lock on 'this'.

Locking on 'this' isn't recommended according to an excellent article by
Jeffrey Richter at http://msdn.microsoft.com/msdnmag/issues/03/01/NET/

Jon Skeet also makes some excellent points about what to lock on at
http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml

Bottom line: create a private variable to lock on and use it only for
synchronization purposes.

Regards,
Mountain

Dan,

I wouldn't use a Mutex. I would just make the method synchronized, like
so:

[MethodImpl(MethodImplOptions.Synchronized)]
public override string MapMessage ( string messageSource )
{
// create and load up a new transform if required
mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
}
else
{
throw ( new ApplicationException ( "Configuration error, XSL file
does not exist. " + configFile));
}

// rest of the function code here
}

Hope this helps.


--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

I know that is wrong! Just realised the problem it could give me...
What about this?


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{

xslt = new XslTransform();
xslt.Load( configFile );

}
else
{
mut.ReleaseMutex();
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

mut.ReleaseMutex();

//
// rest of the function code here

}

"Dan Bass" <danielbass [at] postmaster [dot] co [dot] uk> wrote in message

I know that XslTransform's Transform is thread safe according to the
MSDN, and that Load is not. I've therefore applied this simply Mutex to
it and would just like to confirm this is okay.


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
mut.WaitOne();

xslt = new XslTransform();
xslt.Load( configFile );

mut.ReleaseMutex();
}
else
{
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

//
// rest of the function code here

}

 

[Nicholas Paldino [.NET/C# MVP]]

I've read Richter's article, and I think that the security concerns
don't hold much weight, to be honest. If it got to the point where
malicious code could run on your machine, then whether or not you expose the
lock your objects use (namely, this), doesn't matter. The malicious code
could simply perform a DoS attack using:

while (true);

Now the argument that you only want to lock on a subset of the members
of your class holds more weight, but the counterpoint to that is that with
proper design (where you don't have super sized classes), your operations
will affect the whole class anyways (because they will not be loaded
tremendously with properties and expand the scope of the class by too much).

Also, with reflection, it's almost impossible to truly keep someone from
accessing your locks. You could remove permissions for reflection from the
class, I believe, but then you are hindering yourself in other ways. You
have to decide where to trade off.


--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

Hi Nicholas,
If I understand correctly, using the MethodImpl attribute is the same as a
lock on 'this'.

Locking on 'this' isn't recommended according to an excellent article by
Jeffrey Richter at http://msdn.microsoft.com/msdnmag/issues/03/01/NET/

Jon Skeet also makes some excellent points about what to lock on at
http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml

Bottom line: create a private variable to lock on and use it only for
synchronization purposes.

Regards,
Mountain

in
message news:%[email protected]...

Dan,

I wouldn't use a Mutex. I would just make the method synchronized, like
so:

[MethodImpl(MethodImplOptions.Synchronized)]
public override string MapMessage ( string messageSource )
{
// create and load up a new transform if required
mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
}
else
{
throw ( new ApplicationException ( "Configuration error, XSL file
does not exist. " + configFile));
}

// rest of the function code here
}

Hope this helps.


--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

"Dan Bass" <danielbass [at] postmaster [dot] co [dot] uk> wrote in
message

I know that is wrong! Just realised the problem it could give me...
What about this?


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{

xslt = new XslTransform();
xslt.Load( configFile );

}
else
{
mut.ReleaseMutex();
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

mut.ReleaseMutex();

//
// rest of the function code here

}

"Dan Bass" <danielbass [at] postmaster [dot] co [dot] uk> wrote in message

I know that XslTransform's Transform is thread safe according to the
MSDN, and that Load is not. I've therefore applied this simply Mutex
to
it and would just like to confirm this is okay.


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
mut.WaitOne();

xslt = new XslTransform();
xslt.Load( configFile );

mut.ReleaseMutex();
}
else
{
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

//
// rest of the function code here

}

 

[Helge Jensen]

XslTransform xslt = null;
Mutex mut = new Mutex(); ....
public override string MapMessage ( string messageSource )
{
mut.WaitOne(); ....

mut.ReleaseMutex();
}

You should at least be exception defensive here, using

mut.WaitOne();
try {
...
} finally {
mut.ReleaseMutex();
}

As pointed out other places in this thread, using "lock()" would
probably be preferred to the above:

public override string MapMessage ( string messageSource )
{

lock(someObj) {
....
}

}

Especially since it's less error-prone, and it's very easy to read.

Whether you should lock on "this" or another object, I will leave for a
separate discussion

 

[Mr. Mountain]

You didn't mention one of the most important drawbacks of using lock -- if
the monitor is unavailable, it will block indefinitely (infinite timeout).
I agree with Ian G that production code shouldn't have a "feature" like
this. It seems far better to use Monitor.TryEnter, (or Ian's or Jon Skeet's
code that makes using Monitor.TryEnter as easy as using lock)

I've read Richter's article, and I think that the security concerns
don't hold much weight, to be honest. If it got to the point where
malicious code could run on your machine, then whether or not you expose the
lock your objects use (namely, this), doesn't matter. The malicious code
could simply perform a DoS attack using:

while (true);

Now the argument that you only want to lock on a subset of the members
of your class holds more weight, but the counterpoint to that is that with
proper design (where you don't have super sized classes), your operations
will affect the whole class anyways (because they will not be loaded
tremendously with properties and expand the scope of the class by too much).

Also, with reflection, it's almost impossible to truly keep someone from
accessing your locks. You could remove permissions for reflection from the
class, I believe, but then you are hindering yourself in other ways. You
have to decide where to trade off.


--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

Hi Nicholas,
If I understand correctly, using the MethodImpl attribute is the same as a
lock on 'this'.

Locking on 'this' isn't recommended according to an excellent article by
Jeffrey Richter at http://msdn.microsoft.com/msdnmag/issues/03/01/NET/

Jon Skeet also makes some excellent points about what to lock on at
http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml

Bottom line: create a private variable to lock on and use it only for
synchronization purposes.

Regards,
Mountain

in
message news:%[email protected]...

Dan,

I wouldn't use a Mutex. I would just make the method synchronized, like
so:

[MethodImpl(MethodImplOptions.Synchronized)]
public override string MapMessage ( string messageSource )
{
// create and load up a new transform if required
mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
}
else
{
throw ( new ApplicationException ( "Configuration error, XSL file
does not exist. " + configFile));
}

// rest of the function code here
}

Hope this helps.


--
- Nicholas Paldino [.NET/C# MVP]
- (e-mail address removed)

"Dan Bass" <danielbass [at] postmaster [dot] co [dot] uk> wrote in
message
I know that is wrong! Just realised the problem it could give me...
What about this?


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

mut.WaitOne();

if ( xslt == null )
{
if ( File.Exists(configFile) )
{

xslt = new XslTransform();
xslt.Load( configFile );

}
else
{
mut.ReleaseMutex();
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

mut.ReleaseMutex();

//
// rest of the function code here

}

"Dan Bass" <danielbass [at] postmaster [dot] co [dot] uk> wrote in message

I know that XslTransform's Transform is thread safe according to the
MSDN, and that Load is not. I've therefore applied this simply Mutex
to
it and would just like to confirm this is okay.


XslTransform xslt = null;
Mutex mut = new Mutex();

public override string MapMessage ( string messageSource )
{

//
// create and load up a new transform if required

if ( xslt == null )
{
if ( File.Exists(configFile) )
{
mut.WaitOne();

xslt = new XslTransform();
xslt.Load( configFile );

mut.ReleaseMutex();
}
else
{
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}

//
// rest of the function code here

}

 

[Jon Skeet [C# MVP]]

I've read Richter's article, and I think that the security concerns
don't hold much weight, to be honest. If it got to the point where
malicious code could run on your machine, then whether or not you expose the
lock your objects use (namely, this), doesn't matter. The malicious code
could simply perform a DoS attack using:

while (true);

I don't think it's really a security matter. (The word 'security'
doesn't appear in Richter's article or mine.) For me, it's a matter of
making sure that no-one's code (including my own) will *accidentally*
end up locking on something that another bit of code thinks it's
locking on exclusively.

Using private locks makes the code more readable and more maintainable
- IMO, of course.

 

[Dan Bass]

Bottom line: create a private variable to lock on and use it only for

synchronization purposes.

Thanks for all the responses so far.

Oky, hoolllllld on a minute, threading novice here! What's wrong with Mutex?

 

[Dan Bass]

Reading through your threading pages... going way over this head and feeling
really stupid right now! ;o)

 

[Dan Bass]

Is this better?


readonly object xsltLock = new object();
XslTransform xslt = null;

public override string MapMessage ( string messageSource )
{
//
// create and load up a new transform if required

lock ( xsltLock )
{
if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
else
{
// will lock release on this?
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}
}

//
// rest of the method

}

 

[David Levine]

Looks good; the lock keyword guarantees that the lock will be released
regardless of how control leaves the lock region, so the xlstLock will be
released even if you throw an exception. The issue of whether you should
throw an exception if the file is missing, or what type of exception to
throw, is an issue separate from best practices regarding locks.

Is this better?


readonly object xsltLock = new object();
XslTransform xslt = null;

public override string MapMessage ( string messageSource )
{
//
// create and load up a new transform if required

lock ( xsltLock )
{
if ( xslt == null )
{
if ( File.Exists(configFile) )
{
xslt = new XslTransform();
xslt.Load( configFile );
}
else
{
// will lock release on this?
throw ( new ApplicationException ( "Configuration
error, XSL file does not exist. " + configFile ) );
}
}
}

//
// rest of the method

}

 

[Dan Bass]

David,

Thanks for the feedback.

The Exception is there because the module in which this code exists cannot
function without the presence of the XSL file in question. The desire is to
the catch this exception at a higher level, then log the details alerting
the user of this incident. The application is a windows service using a
plug-in archtecture. This code belongs to one such plug-in.

I assumed it would be an ApplicationException simlpy because it's a general
error thrown by the application code.

Is my thinking correct on these issues?

Looks good; the lock keyword guarantees that the lock will be released
regardless of how control leaves the lock region, so the xlstLock will be
released even if you throw an exception. The issue of whether you should
throw an exception if the file is missing, or what type of exception to
throw, is an issue separate from best practices regarding locks.

 

[David Levine]

David,

Thanks for the feedback.

The Exception is there because the module in which this code exists cannot
function without the presence of the XSL file in question. The desire is
to the catch this exception at a higher level, then log the details
alerting the user of this incident. The application is a windows service
using a plug-in archtecture. This code belongs to one such plug-in.

I assumed it would be an ApplicationException simlpy because it's a
general error thrown by the application code.

Is my thinking correct on these issues?

This appears to be a case where throwing is the correct course of action.

There is no clear agreement on what exception type ought to be thrown. If
there is a programmatic action that a caller can take to recover from the
exception then creating a custom exception type is useful - the caller can
then catch that specific type and take corrective action. If there is no
clearly defined recovery mechanism then using a generic exception is the
preferred method.

The latest guidelines suggest that using ApplicationException is to be
discouraged as the value-add is small, but if there is no predefined
exception type that is suitable then the other option is to simply throw the
base Exception type. I have no yet reached a conclusion as to which I
prefer, although for simplicity I typically use the base Exception class.

 

[Dan Bass]

The latest guidelines suggest that using ApplicationException is to be

discouraged as the value-add is small, but if there is no predefined
exception type that is suitable then the other option is to simply throw
the base Exception type. I have no yet reached a conclusion as to which I
prefer, although for simplicity I typically use the base Exception class.

I used to too, but then tried a trial of devAdvantage said they should
rather be ApplicationException! I'm not too fussed really because the result
isn't too different in terms of performance or readability.

 

[Jon Skeet [C# MVP]]

Thanks for all the responses so far.

Oky, hoolllllld on a minute, threading novice here! What's wrong with
Mutex?

1) It's inefficient compared with monitors - mutexes are much heavier
than monitors, and aren't as well understood by the CLR, so it can't
optimise around them.

2) It doesn't have language support, so it's harder to use.

 

[Dan Bass]

if i'm using the lock(localObject) mechanism, will that lock it from other
processes running as well, or just threads within the same process?

 

[Richard Blewett [DevelopMentor]]

Nope, Monitors are AppDomain specific.If you want to perform cross AppDomain (including cross process) you will need to use Mutexes, Events and other such kernel objects

Regards

Richard Blewett - DevelopMentor
http://www.dotnetconsult.co.uk/weblog
http://www.dotnetconsult.co.uk

if i'm using the lock(localObject) mechanism, will that lock it from other
processes running as well, or just threads within the same process?

 

[Willy Denoyette [MVP]]

Nope, Monitors are AppDomain specific.If you want to perform cross
AppDomain (including cross process) you will need to use Mutexes, Events
and other such kernel objects

Regards

Richard Blewett - DevelopMentor
http://www.dotnetconsult.co.uk/weblog
http://www.dotnetconsult.co.uk

if i'm using the lock(localObject) mechanism, will that lock it from
other
processes running as well, or just threads within the same process?

Richard,

IMO Monitors themselves are not Domain "specific", all depends on the type
you are taking a lock on. If you take a lock on a domain neutral type, you
will get a cross-domain lock, else you get a domain specific lock.

Willy.