Is this Threadsafe?

  • Thread starter Thread starter Gina_Marano
  • Start date Start date
G

Gina_Marano

public partial class FormMain : Form
{
private static Object LockVariable = new Object();
public static SpecXMLFileNameCache aFileCache = new
SpecXMLFileNameCache();

public class SpecXMLFileNameCache
{
private Dictionary<string, bool> _SpecXMLFileNameCache;

public SpecXMLFileNameCache()
{
_SpecXMLFileNameCache = new Dictionary<string, bool>
();
}

public void AddXMLFileName(string aFileName)
{
lock(LockVariable)
{
_SpecXMLFileNameCache.Add(aFileName,false);
}
}

public void UpdateXMLFileNameStatus(string aFileName)
{
lock(LockVariable)
{
if (_SpecXMLFileNameCache.ContainsKey(aFileName))
_SpecXMLFileNameCache[aFileName] = true;
}
}

public bool ContainsXMLFileName(string aFileName)
{
lock (LockVariable)
{
bool bAlreadyObtained = false;
_SpecXMLFileNameCache.TryGetValue(aFileName, out
bAlreadyObtained);
return bAlreadyObtained;
}
}
}
....

private void btnAddSpecXMLFileName_Click(object sender,
EventArgs e)
{
if (eAddSpecXMLFileName.Text.Trim() != "")
{
aFileCache.AddXMLFileName(eAddSpecXMLFileName.Text);
}
}
}
}

The user will add items to this list via the GUI. A background thread
is running that will have an external list of files. It will see if
the file name is in the list above and hasn't yet been downloaded. If
this is the case it will download the file and update the item in the
list as downloaded.

Are there any threading issues here?

Thanks

~Gina_M~
 
Gina_Marano said:
public partial class FormMain : Form
{
private static Object LockVariable = new Object();
public static SpecXMLFileNameCache aFileCache = new
SpecXMLFileNameCache();

public class SpecXMLFileNameCache
{
private Dictionary<string, bool> _SpecXMLFileNameCache;

public SpecXMLFileNameCache()
{
_SpecXMLFileNameCache = new Dictionary<string, bool>
();
}

public void AddXMLFileName(string aFileName)
{
lock(LockVariable)
{
_SpecXMLFileNameCache.Add(aFileName,false);
}
}

public void UpdateXMLFileNameStatus(string aFileName)
{
lock(LockVariable)
{
if (_SpecXMLFileNameCache.ContainsKey(aFileName))
_SpecXMLFileNameCache[aFileName] = true;
}
}

public bool ContainsXMLFileName(string aFileName)
{
lock (LockVariable)
{
bool bAlreadyObtained = false;
_SpecXMLFileNameCache.TryGetValue(aFileName, out
bAlreadyObtained);
return bAlreadyObtained;
}
}
}
...

private void btnAddSpecXMLFileName_Click(object sender,
EventArgs e)
{
if (eAddSpecXMLFileName.Text.Trim() != "")
{
aFileCache.AddXMLFileName(eAddSpecXMLFileName.Text);
}
}
}
}

The user will add items to this list via the GUI. A background thread
is running that will have an external list of files. It will see if
the file name is in the list above and hasn't yet been downloaded. If
this is the case it will download the file and update the item in the
list as downloaded.

Are there any threading issues here?
Click to expand...

Since all the methods uses a lock for everything they do, then
I would say that single calls are indeed thread safe.

No guarantee for multiple calls.

And I do not like the design with:
- non static methods locking on static object
- public static field

Arne
 
Of course, that's an issue with most "thread-safe" classes. But I agree,
it's worth pointing out, especially to someone who needs to ask if their
code is thread-safe. :)
Click to expand...

Ouch! ;)
All that said, the entire design seems a little "off" to me. For this
particular usage, it seems to me that simply maintaining the dictionary
inside the FormMain class, and using the usual Control.Invoke() mechanism
to isolate access to the dictionary to a single thread, would be the
simplest, most reliable implementation. It's doubtful that the access to
the dictionary will be done with such frequency that there are any
performance issues to be concerned with, and using Control.Invoke() allows
the code to inherit the synchronization implicit in running all the
relevant code on a single thread.
Click to expand...

You are probably correct and a very good idea! I was all wrapped up in
the locking object and not thinking of this way!

Thanks all!

~Gina_M~
 
Ok, can I get a little help here since I have shown to be threadsafe
inadequate... :)

I have used Control.Invoke() in the past but that was to just update
the GUI.

Not sure how to use Control.Invoke() with different functionalities
acting on the same object.

Do I make one super delegate with a set of params/flags to specify
what I want to do?

Three functionalities that I need:

1) add an item to the dictionary from the GUI

2) check to see if the is in the dictionary (from a thread)
3) update the dictionary value from a thread (same thread as #2)

2 and 3 will never be concurrent.

~Gina_M~
 
-- Invoke() always returns an Object, which has to be cast back to the
actual return type of the invoked method.

-- I've switched your code to use Contains() instead of TryGetValue();
the latter wasn't useful, because you never did anything with the
retrieved value.

-- You could separate the call to Contains() into a different method,
but because it's specifically tied to this method, I prefer to use an
anonymous method.
Click to expand...

No, you absolutely should use TryGetValue, and you should return the value
at the same time. Otherwise there is a race condition between finding out
the value exists and then trying to retrieve it (oops, someone removed it in
between).
 
Peter Duniho said:
rOn Fri, 24 Apr 2009 09:19:52 -0700, Ben Voigt [C++ MVP]
-- I've switched your code to use Contains() instead of
TryGetValue(); the latter wasn't useful, because you never did anything
with the retrieved value.
Click to expand...
[...]

No, you absolutely should use TryGetValue, and you should return the
value at the same time. Otherwise there is a race condition between
finding out the value exists and then trying to retrieve it (oops,
someone removed it in between).
Click to expand...

Oops. I misread the code, and overlooked that the retrieved value was
what was being returned, not the success/failure of TryGetValue().

Of course, you misread my post and overlooked the reason I stated for
changing from TryGetValue() to Contains(). So, I have that small
consolation. :)

And, the race condition you describe doesn't exist for synchronized code
(as we have here). In particular, if I _had_ reimplemented the exact
behavior, but with Contains() and a value lookup, it would have worked
fine. There's no way for any other code to modify the Dictionary until
that method returns.
Click to expand...

As long as it's in the delegate being invoked across threads, yes. It's the
overall method that needs to have the TryGetValue behavior, the
implementation doesn't matter.

The race condition I was referring to was:

result = Invoke({method that calls Contains)};
if (result) Invoke({method that acts on the value});

Since you said there was such a method that calls Contains and doesn't act
on the value, and was being Invoke-d... I saw a problem.
But even so, I agree that since we need the actual value, not just the
present/not-present state, TryGetValue() as originally used is superior.
Click to expand...

I never looked at the code you were commenting on... I was just commenting
on the fallacy of ever checking on presence using a separate marshaled call.
 
Peter Duniho said:
[...]
And, the race condition you describe doesn't exist for synchronized
code (as we have here). In particular, if I _had_ reimplemented the
exact behavior, but with Contains() and a value lookup, it would have
worked fine. There's no way for any other code to modify the
Dictionary until that method returns.
Click to expand...

As long as it's in the delegate being invoked across threads, yes. It's
the overall method that needs to have the TryGetValue behavior, the
implementation doesn't matter.

The race condition I was referring to was:

result = Invoke({method that calls Contains)};
if (result) Invoke({method that acts on the value});
Click to expand...

There's no code that does that, as far as I know. Did you see some?
[...]
But even so, I agree that since we need the actual value, not just the
present/not-present state, TryGetValue() as originally used is superior.
Click to expand...

I never looked at the code you were commenting on... I was just
commenting on the fallacy of ever checking on presence using a separate
marshaled call.
Click to expand...

I never suggested in my post that the code do that. I'm not sure where
you're getting that.

It's true that I incorrectly reimplemented the OP's ContainsXMLFileName()
method. But it was a simple functional error; there aren't any race
conditions, newly introduced or otherwise.
Click to expand...

This function, which you posted, has a race:

public bool ContainsXMLFileName(string aFileName)
{
return (bool)Invoke((Func<bool>) delegate
{
return _SpecXMLFileNameCache.Contains(aFileName);
});
}

return Cache.Contains()
and
return (bool)Invoke()

run on two different threads. The thread on which Cache.Contains executes
will go on to perform more processing, possibly including
Cache.Remove(aFileName). And then the ContainsXMLFileName thread goes from
ready to running and returns the wrong answer.
 
Peter Duniho said:
[...]
It's true that I incorrectly reimplemented the OP's
ContainsXMLFileName() method. But it was a simple functional error;
there aren't any race conditions, newly introduced or otherwise.
Click to expand...

This function, which you posted, has a race:

public bool ContainsXMLFileName(string aFileName)
{
return (bool)Invoke((Func<bool>) delegate
{
return _SpecXMLFileNameCache.Contains(aFileName);
});
}

return Cache.Contains()
and
return (bool)Invoke()

run on two different threads. The thread on which Cache.Contains
executes will go on to perform more processing, possibly including
Cache.Remove(aFileName). And then the ContainsXMLFileName thread goes
from ready to running and returns the wrong answer.
Click to expand...

Sorry. I have no idea what you're talking about.

If I assume you mean "_SpecXMLFileNameCache.Contains()" when you write
Yes.

"Cache.Contains()", the thread that calls that -- the main GUI thread --
doesn't "go on to perform more processing" any more than it ever did in
the OP's original code. In the specific code you've quoted, the invoked
method calls the Contains() method and then immediately returns; control
Click to expand...

It returns to the main message loop, and can do basically anything next,
agreed?
is then returned to the thread that called Invoke(), where the return
value is retrieved.
Click to expand...

No, control is not returned to the thread that called Invoke. The thread
that called Invoke changed from blocking to ready, but there is no reason to
believe it continues running immediately. A dozen more messages could be
processed in the main GUI thread before its timeslice is up and the other
thread retrieves and uses the return value.
The fact that the main GUI thread is free to continue doing something else
is immaterial to the code I posted. I did warn at the outset against
operations that may rely on a sequence of calls to the synchronized
methods. For the remainder of this message thread, there's no reason to
continue reminding of that particular issue, nor IMHO is there any reason
to believe that the code would eventually be used in a way that such a
race condition would be an issue.
Click to expand...

I think there's a difference between code that could be used wrongly, and
code that cannot be used correctly. The version of ContainsXMLFileName
which functions like List.Contains cannot be used without introducing a race
condition. The value returned reflects the state of the program at some
earlier time, not the current state of the program. By the time you get the
value, it's already too late to do anything useful with it.

I finally found the blog post which I saw a while back that explains all of
this in gory detail. In fact, the poster who contemplates having a
threadsafe list should definitely read this:
http://blogs.msdn.com/jaredpar/archive/2009/02/16/a-more-usable-thread-safe-collection.aspx
 
The version of ContainsXMLFileName which functions like List.Contains
Sure it can. It just depends on how you're using it.


First of all, you can't say that with authority unless you know why the
value returned by "Contains()" is useful. If a subsequent operation on
the cache is only valid given a certain return value, and code that calls
the ContainsXMLFileName() method relies on that return value to determine
whether to perform that operation, your statement is correct.

But if (for example) the value is simply used as a momentary snapshot of
the current state of the cache, or if further synchronization is done with
Click to expand...

But it isn't the current state of the cache anymore, by the time Invoke
returns to the calling thread. It's a past state. Ok, it is possible that
knowing that the cache held that value "once upon a time" is useful.
methods that may modify the cache, there is no problem.

I'm not saying that code that uses this design would be the best design.
But you're wrong to state that there's no way to use the code correctly.
Click to expand...

Point taken. If you add locking around the allegedly thread-safe accessors,
you could avoid the race condition. But what's the point of using Invoke at
all then? The code was definitely faulty as written, the combination of
Invoke+Contains has no advantages whatsoever over Invoke alone.
More importantly: your whole complaint seems to be centered around the use
of the method Contains(), which I already acknowledged is an incorrect
representation of the original code's logic. In other words, the code I
posted is only properly viewed by assuming we correct the mistake I made
in the conversion from the original code. Once you do that, we're back to
using TryGetValue(), and the complaint about using Contains() vanishes.
Click to expand...

Agreed. Invoke+TryGetValue does add thread-safety.
 
Thanks Peter and Ben for being so helpful!!!!

They need little donate buttons or something on these newsgroups.
ehhehe

Much, much appreciated!

~Gina_M~
 
Back
Top