Threading Question

D

DBC User

I have a background process which reads a table to see if there are any
pending requests. If there are any, then it will start a worker thread
(only 10 allowed at a time) and executes a method. In this method, I
iniate a PROCESS and on completion, it reduces the available worker
thread and continue.

I have couple of questions;
1. Since I am launching multiple threads on a same method, do I have to
take care of locking so that each one doesn't step on each other or the
thread will creates its own address space and run the method there so I
need not worry about any locking. All I do in that method is, when the
method completes I reset the status of the request as complete or free.
2. Currently I am doing locking as you can see in the code I am adding
at the end.

Thanks for the input.

internal void ProcessRequests()
{
BatchRequests bProc = new BatchRequests();

while (true)
{
if (bProc.AnyRowAvailable())
{
bProc.Rewind();
do
{
if (bProc.LastAttempt.AddMinutes(15) <
DateTime.Now)
{
while (workingThread > 11)
System.Threading.Thread.Sleep(5000);

System.Threading.ThreadPool.QueueUserWorkItem(ProcessOneRequest,
(object)bProc);
UpdateAsInUse(bProc.AppName,
bProc.AppVersion);
System.Threading.Thread.Sleep(5000);
workingThread++;
}
} while (bProc.MoveNext());
}
else
{
System.Threading.Thread.Sleep(60000);
}
bProc = null;
bProc = new TaxAppBatchRequests();
}
}

protected void ProcessOneRequest(object request)
{
lock (request)
{
BatchRequests proc = (BatchRequests)request;
if (proc.ObjectImage.Length > 0)
{
Request req = (Request)request;
if (req.ProcessRequest())
UpdateAsComplete(proc.AppName,
proc.AppVersion);
else
UpdateAsFree(proc.AppName, proc.AppVersion);
}
workingThread--;
}
}
 
B

Bruce Wood

DBC said:
I have a background process which reads a table to see if there are any
pending requests. If there are any, then it will start a worker thread
(only 10 allowed at a time) and executes a method. In this method, I
iniate a PROCESS and on completion, it reduces the available worker
thread and continue.

I have couple of questions;
1. Since I am launching multiple threads on a same method, do I have to
take care of locking so that each one doesn't step on each other or the
thread will creates its own address space and run the method there so I
need not worry about any locking. All I do in that method is, when the
method completes I reset the status of the request as complete or free.
2. Currently I am doing locking as you can see in the code I am adding
at the end.

Thanks for the input.

internal void ProcessRequests()
{
BatchRequests bProc = new BatchRequests();

while (true)
{
if (bProc.AnyRowAvailable())
{
bProc.Rewind();
do
{
if (bProc.LastAttempt.AddMinutes(15) <
DateTime.Now)
{
while (workingThread > 11)
System.Threading.Thread.Sleep(5000);

System.Threading.ThreadPool.QueueUserWorkItem(ProcessOneRequest,
(object)bProc);
UpdateAsInUse(bProc.AppName,
bProc.AppVersion);
System.Threading.Thread.Sleep(5000);
workingThread++;
}
} while (bProc.MoveNext());
}
else
{
System.Threading.Thread.Sleep(60000);
}
bProc = null;
bProc = new TaxAppBatchRequests();
}
}

protected void ProcessOneRequest(object request)
{
lock (request)
{
BatchRequests proc = (BatchRequests)request;
if (proc.ObjectImage.Length > 0)
{
Request req = (Request)request;
if (req.ProcessRequest())
UpdateAsComplete(proc.AppName,
proc.AppVersion);
else
UpdateAsFree(proc.AppName, proc.AppVersion);
}
workingThread--;
}
}
Click to expand...

It's difficult to tell without the rest of the code (code for
BatchRequests, for example, and what type is "workingThread"), but I
see a few potential issues with this code.

1. Why do you UpdateAsInUse(bProc.AppName, bProc.AppVersion); _after_
you start the worker thread? This strikes me as backward. Remember that
timing between threads is arbitrary, so it is theoretically possible
that the worker thread could run to completion before you get to the
line that says "UpdateAsInUse"... so your worker thread would
UpdateAsComplete, and _then_ your main thread would UpdateAsInUse.
Probably not what you want. Change the status of the request, _then_
start the worker thread.

2. Given this change, I don't see what the
System.Threading.Thread.Sleep(5000); immediately after that is buying
you. There should be no need to sleep if you've set the status of the
request to "in use". You should be able to loop right back to the top
and look for another request to process.

3. Also, given all of this, I see no reason to lock on the request
itself. Once you've marked it "in use", I would hope that it's no
longer a candidate for processing, so there's no danger of picking it
again or colliding on it.

4. Where there is danger of collision is on the workingThread variable.
There is no guarantee (that I know of) that workingThread++ and
workingThread-- are atomic operations. I believe that if one thread is
incrementing the variable while another is decrementing it, you could
get unpredictable behaviour. You may not have to resort to a lock,
though: there may be a way to do simple increments / decrements in a
thread-safe manner, although it will take someone smarter than I am to
say how to do that.

5. Without the code for BatchRequests I can't be sure, but the fact
that bProc seems to encapsulate both a current position your list and
an item to process. I'm almost positive that looping back around,
saying bProc.Rewind() or bProc.MoveNext() and then looking for another
request to process will affect which request is returned by
(Request)bProc, in which case you need to change that class, because
your main thread could loop back to the top and say bProc.MoveNext() or
bProc.Rewind() before the worker thread has a chance to grab the
pointer to the request that it is to process. I think that you need
something more like this:

bProc.Rewind();
do
{
Request r = bProc.CurrentRequest;
if (r.LastAttempt.AddMinutes(15) < DateTime.Now)
{
while (workingThread > 11)
System.Threading.Thread.Sleep(5000);
UpdateAsInUse(r.AppName, r.AppVersion);

System.Threading.ThreadPool.QueueUserWorkItem(ProcessOneRequest, r);
workingThread++;
}
} while (bProc.MoveNext());

so that moving the bProc "current request" pointer doesn't affect the
Request pointers held by any active worker threads.
 
D

DBC User

Hi Bruce,
Thanks and it makes perfect sense why should I be making it in use
after the thread. I will make the change. But one question though, I am
calling a function to execute a process and it done 3 times and it
looks like only the last one get updated while other 2 run but its
status doesn't get updates.
Should I be using method instance instead of calling this method
stright?
Thanks again.
 
B

Bruce Wood

I'm not sure that I understand what you're trying to explain, but could
the problem be that what you pass to the background thread is sort of a
"current request" pointer instead of the request itself, and then the
loop moves the "current request" on to the next one and starts another
thread?

I'm not sure. I would have to see more code, and a clearer description
of exactly what is happening, to be able to help further.
 
D

DBC User

HI Bruce,
Thanks, it looks like I was passing the same object over and over and
the last object stuck in the thread.
THanks.
 
Top