Prev: DSOFramer Successor
Next: issue when using functions (having CString& type parameter) from C++ dll in to C#
From: David on 17 Dec 2009 11:41
Hi All (and Peter),

I have my WIA stuff working now with info I eventually found on the net.

I have sucessfully put it into a producer/consumer thread that Peter
mentioned earlier this month. I went to one of the sites mentioned in the
message (actually, I went to both)...

http://www.yoda.arachsys.com/csharp/threads/deadlocks.shtml

About half way down is "More monitor deadlocks" which has an example of the
producer / consumer.

I am using that, but I am finding when I close my application, it is still
running...

Basically, in my consumer thread, I have a loop...

while (AppRunning)
{
object o = queue.Consume();

// Rest of my code.
}

The AppRunning bool is set to false when I close my app, but as the consumer
is holding at the queue.Consume(); the AppRunning can never be checked.

So, here is what I am thinking...

Can I make
Queue queue = new Queue();

in the ProducerConsumer a public item? If I can, then in my close, I can
check if the queue.Count is zero and if it is, I should be able to run
Thread.Abort. I guess that I will have to loop through the queue.Count ...
while (!queue.Count = 0) until it has completed.

Alternatively, is there a better way of doing this?


As I expect the consumer to take a while to clear, I want to ensure it is
empty before it is finally destroyed.

--
Best regards,
Dave Colliver.
http://www.AshfieldFOCUS.com
~~
http://www.FOCUSPortals.com - Local franchises available



From: Peter Duniho on 17 Dec 2009 12:45
David wrote:
> [...]
> Can I make
> Queue queue = new Queue();
>
> in the ProducerConsumer a public item? If I can, then in my close, I can
> check if the queue.Count is zero and if it is, I should be able to run
> Thread.Abort. I guess that I will have to loop through the queue.Count ...
> while (!queue.Count = 0) until it has completed.
>
> Alternatively, is there a better way of doing this?

Don't abort the thread. Even if you were careful, and only aborted the
thread while the thread calling Thread.Abort() has the lock, to ensure
no one else tries to queue more items while you're aborting the consumer
thread, you could find that you're accidentally aborting the thread
while it's processing something it's already removed from the queue
(i.e. making the count 0).

It's also just not a good habit to get into. It's too easy to create
data consistency/corruption bugs by aborting threads.

There's also the issue of exposing the queue instance, which isn't a
very good idea (it's encapsulated into the ProducerConsumer class for a
reason), but of course you could address that by just wrapping the
inspection of the count in a property or something.

Jon's sample code is really just that: sample code. I don't think he
intended it to be used as full production code, but rather just to
illustrate the basic concept. So it may be missing features useful to you.

In this case, you have at least a couple of other options that are much
better than aborting the thread:

-- Keep a flag, and wake the consumer up when you need it to check it
-- Enqueue a sentinel value (e.g. "null") that the consumer can watch for

Either should be fine. The latter is probably easier to incorporate
into your code if you've already followed Jon's example. The former is
potentially more reliable, because it provides a flag that producers can
also check and generate an error if they try to enqueue something after
shutdown has started (which may or may not be an issue in your case...if
you are only shutting down after there's no possibility of a producer
adding something, it's not an issue).

> As I expect the consumer to take a while to clear, I want to ensure it is
> empty before it is finally destroyed.

With either of the above suggestions, you can leave it up to the
consumer thread to terminate itself (by simply exiting when it's done).

Pete
From: David on 17 Dec 2009 13:32
Thanks, I think I have got it...

I have created a public bool property in the ProducerConsumer. This is
called Terminate and is set to false by default.

From my form closing, I set the Terminate to true, I then do
queue.Produce(null) which triggers the pulse. In my consumer, I have added
to the while statement... while (queue.Count == 0 && !Terminated) (I think I
need to change this actually, I will work on the logic.)

In my uploader code (which is what the consumer thread is handling) I am
checking for null before I do an upload (just in case).

I think what I have done is the first option you have said, though it sounds
like a combination.
--
Best regards,
Dave Colliver.
http://www.AshfieldFOCUS.com
~~
http://www.FOCUSPortals.com - Local franchises available


"Peter Duniho" <no.peted.spam(a)no.nwlink.spam.com> wrote in message
news:O8TaLD0fKHA.1652(a)TK2MSFTNGP05.phx.gbl...
> David wrote:
>> [...]
>> Can I make
>> Queue queue = new Queue();
>>
>> in the ProducerConsumer a public item? If I can, then in my close, I can
>> check if the queue.Count is zero and if it is, I should be able to run
>> Thread.Abort. I guess that I will have to loop through the queue.Count
>> ... while (!queue.Count = 0) until it has completed.
>>
>> Alternatively, is there a better way of doing this?
>
> Don't abort the thread. Even if you were careful, and only aborted the
> thread while the thread calling Thread.Abort() has the lock, to ensure no
> one else tries to queue more items while you're aborting the consumer
> thread, you could find that you're accidentally aborting the thread while
> it's processing something it's already removed from the queue (i.e. making
> the count 0).
>
> It's also just not a good habit to get into. It's too easy to create data
> consistency/corruption bugs by aborting threads.
>
> There's also the issue of exposing the queue instance, which isn't a very
> good idea (it's encapsulated into the ProducerConsumer class for a
> reason), but of course you could address that by just wrapping the
> inspection of the count in a property or something.
>
> Jon's sample code is really just that: sample code. I don't think he
> intended it to be used as full production code, but rather just to
> illustrate the basic concept. So it may be missing features useful to
> you.
>
> In this case, you have at least a couple of other options that are much
> better than aborting the thread:
>
> -- Keep a flag, and wake the consumer up when you need it to check it
> -- Enqueue a sentinel value (e.g. "null") that the consumer can watch
> for
>
> Either should be fine. The latter is probably easier to incorporate into
> your code if you've already followed Jon's example. The former is
> potentially more reliable, because it provides a flag that producers can
> also check and generate an error if they try to enqueue something after
> shutdown has started (which may or may not be an issue in your case...if
> you are only shutting down after there's no possibility of a producer
> adding something, it's not an issue).
>
>> As I expect the consumer to take a while to clear, I want to ensure it is
>> empty before it is finally destroyed.
>
> With either of the above suggestions, you can leave it up to the consumer
> thread to terminate itself (by simply exiting when it's done).
>
> Pete


From: Peter Duniho on 17 Dec 2009 16:27
David wrote:
> Thanks, I think I have got it...
>
> I have created a public bool property in the ProducerConsumer. This is
> called Terminate and is set to false by default.
>
> From my form closing, I set the Terminate to true, I then do
> queue.Produce(null) which triggers the pulse. In my consumer, I have added
> to the while statement... while (queue.Count == 0 && !Terminated) (I think I
> need to change this actually, I will work on the logic.)
>
> In my uploader code (which is what the consumer thread is handling) I am
> checking for null before I do an upload (just in case).
>
> I think what I have done is the first option you have said, though it sounds
> like a combination.

The description of your code makes it seem like it may be overly
complicated, and perhaps also not quite correct. Did you synchronize
the assignment to the boolean property in any way (e.g. "volatile", or
an actual lock)? In Jon's code, the consumer thread is actually
implemented by the client, not the ProducerConsumer class; does your
client code correctly detect the termination condition and exit?

Using my second suggestion, the _only_ change you should need to make is
in your own consumer thread code (i.e. the code that calls Jon's
ProducerConsumer.Consume() method), check for a null return value and
exit the thread when it's null (otherwise, consume the value as normal).
You shouldn't need the sentinel _and_ a flag.

If for some reason, you see the use of a flag more desirable or
important (e.g. you need to block producers from adding to work after
shutdown has initiated), then IMHO it makes more sense to move the
consumer thread into the ProducerConsumer class itself, because of the
additional logic that is required to be in each consumer thread.

With that in mind, see below for a modification of Jon's sample that you
might find a useful example to work from if you would prefer to use my
first suggestion.

I suspect he wrote the example before C# had generics and anonymous
methods...in any case, I've modified it to take advantage of both. I've
also made some tweaks to the testing code to help illustrate better the
specific behavior of the implementation:

-- Increasing the max time for a consumer to work on an item
from 1000 ms to 2000 ms (twice the max for the producer) helps
show that the consumer may lag behind the producer without
problem.

-- Adding a second consumer helps show that, with variations
between processing time, tasks are not necessarily completed
in the same order in which they are enqueued (there is in fact
a very tiny chance they won't even be started in the same
order in which they are enqueued, but this is so dependent on
the exact thread scheduling that without a bunch of extra
synchronization code to force the issue, you'd probably almost
never see it happen).

-- Move the consumer's console output line to the end of its
"processing" rather than the beginning, to better illustrate
the above two points (especially the second).

Finally, the biggest change I made to Jon's example is to, as I
suggested above, have the ProducerConsumer class itself manage the
consumer threading, with the client code only providing an event handler
for the actual consumption of an item in the queue.

Here's the code:


using System;
using System.Collections.Generic;
using System.Threading;

public class Test
{
static void Main()
{
Random rng = new Random(0);

using (ProducerConsumer<int> queue =
new ProducerConsumer<int>())
{
queue.Consume += (i) =>
{
// Note: the call to Thread.Sleep() simply
// simulates a consumer that takes variable
// time to do some work. An actual consumer
// would not include that.
Thread.Sleep(rng.Next(2000));
Console.WriteLine("\t\t\t\tConsumed {0}", i.ToString());
};

// Call this multiple times for multiple consumers,
// or just once for only a single consumer. Creating
// multiple consumers can be very helpful if the data
// for each queue task is 100% isolated, without
// shared resources between tasks. Otherwise, be sure
// to mind the synchronization, and the use of resources
// to minimize contention (e.g. don't create more
// consumers for uploading files than you have bandwidth
// to support).
queue.StartConsumer();
queue.StartConsumer();

for (int i = 0; i < 10; i++)
{
Console.WriteLine("Producing {0}", i);
queue.Produce(i);
Thread.Sleep(rng.Next(1000));
}
}

Console.ReadLine();
}
}

public class ProducerConsumer<T> : IDisposable
{
readonly object listLock = new object();
readonly Queue<T> queue = new Queue<T>();
bool fTerminate;

public event Action<T> Consume = delegate { };

public void StartConsumer()
{
new Thread(_ConsumerThread).Start();
}

public void StopAllConsumers()
{
lock (listLock)
{
fTerminate = true;
Monitor.PulseAll(listLock);
}
}

public void Produce(T t)
{
lock (listLock)
{
if (fTerminate)
{
throw new InvalidOperationException(
"Consumers are shutting down");
}

queue.Enqueue(t);

Monitor.Pulse(listLock);
}
}

public void _ConsumerThread()
{
while (true)
{
T t = default(T);

lock (listLock)
{
while (queue.Count == 0)
{
if (fTerminate)
{
return;
}
Monitor.Wait(listLock);
}

t = queue.Dequeue();
}

Consume(t);
}
}

#region IDisposable Members

public void Dispose()
{
Dispose(true);
GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool fDisposing)
{
if (fDisposing)
{
// dispose any managed resources here.
// for now, none are present in this
// implementation
}

StopAllConsumers();
}

~ProducerConsumer()
{
Dispose(false);
}

#endregion
}
From: David on 18 Dec 2009 07:48
Right, I think I have it now...

I have put the "using" inside my button click to only start the thread when
the button is clicked.

I have made my ScanAndSaveOnePage return a string (was a void), the string
contains the file details that I want to push to the webserver. I have
modified my PushToWebServer to receive a string.

So, inside my button click is...

using (ProducerConsumer<string> queue = new ProducerConsumer<string>())
{
queue.Consume += (i) =>
{
PushToWebServer(i);
};

queue.StartConsumer();

WantsToScan = true;
while (WantsToScan)
{
queue.Produce(ScanAndSaveOnePage());
}
}

(Because my app is for scanning, the gui becomes unresponsive, which is
fine, as I don't want the users clicking other buttons to try and scan
something else whilst it is scanning)

I noted that yours was inside the Main event, which I understand is for a
console application.

I have never used => and at the moment, I don't understand the queue.Consume
+= (i) => { do action }; (actually, just laying it out like that makes it
look like an array, so this must be a generics thing, but what does =>
mean)?

This threading stuff is very frustrating, each time I apply threading, my
app breaks. I have to undo it and it is still broken, so I fix the problem
then re-apply threading. It is highlighting other issues, but it is only the
threading that is highlighting it. It seems like a whole different
discipline in programming.

--
Best regards,
Dave Colliver.
http://www.AshfieldFOCUS.com
~~
http://www.FOCUSPortals.com - Local franchises available


"Peter Duniho" <no.peted.spam(a)no.nwlink.spam.com> wrote in message
news:eaP96%231fKHA.2104(a)TK2MSFTNGP05.phx.gbl...
> David wrote:
>> Thanks, I think I have got it...
>>
>> I have created a public bool property in the ProducerConsumer. This is
>> called Terminate and is set to false by default.
>>
>> From my form closing, I set the Terminate to true, I then do
>> queue.Produce(null) which triggers the pulse. In my consumer, I have
>> added to the while statement... while (queue.Count == 0 && !Terminated)
>> (I think I need to change this actually, I will work on the logic.)
>>
>> In my uploader code (which is what the consumer thread is handling) I am
>> checking for null before I do an upload (just in case).
>>
>> I think what I have done is the first option you have said, though it
>> sounds like a combination.
>
> The description of your code makes it seem like it may be overly
> complicated, and perhaps also not quite correct. Did you synchronize the
> assignment to the boolean property in any way (e.g. "volatile", or an
> actual lock)? In Jon's code, the consumer thread is actually implemented
> by the client, not the ProducerConsumer class; does your client code
> correctly detect the termination condition and exit?
>
> Using my second suggestion, the _only_ change you should need to make is
> in your own consumer thread code (i.e. the code that calls Jon's
> ProducerConsumer.Consume() method), check for a null return value and exit
> the thread when it's null (otherwise, consume the value as normal). You
> shouldn't need the sentinel _and_ a flag.
>
> If for some reason, you see the use of a flag more desirable or important
> (e.g. you need to block producers from adding to work after shutdown has
> initiated), then IMHO it makes more sense to move the consumer thread into
> the ProducerConsumer class itself, because of the additional logic that is
> required to be in each consumer thread.
>
> With that in mind, see below for a modification of Jon's sample that you
> might find a useful example to work from if you would prefer to use my
> first suggestion.
>
> I suspect he wrote the example before C# had generics and anonymous
> methods...in any case, I've modified it to take advantage of both. I've
> also made some tweaks to the testing code to help illustrate better the
> specific behavior of the implementation:
>
> -- Increasing the max time for a consumer to work on an item
> from 1000 ms to 2000 ms (twice the max for the producer) helps
> show that the consumer may lag behind the producer without
> problem.
>
> -- Adding a second consumer helps show that, with variations
> between processing time, tasks are not necessarily completed
> in the same order in which they are enqueued (there is in fact
> a very tiny chance they won't even be started in the same
> order in which they are enqueued, but this is so dependent on
> the exact thread scheduling that without a bunch of extra
> synchronization code to force the issue, you'd probably almost
> never see it happen).
>
> -- Move the consumer's console output line to the end of its
> "processing" rather than the beginning, to better illustrate
> the above two points (especially the second).
>
> Finally, the biggest change I made to Jon's example is to, as I suggested
> above, have the ProducerConsumer class itself manage the consumer
> threading, with the client code only providing an event handler for the
> actual consumption of an item in the queue.
>
> Here's the code:
>
>
> using System;
> using System.Collections.Generic;
> using System.Threading;
>
> public class Test
> {
> static void Main()
> {
> Random rng = new Random(0);
>
> using (ProducerConsumer<int> queue =
> new ProducerConsumer<int>())
> {
> queue.Consume += (i) =>
> {
> // Note: the call to Thread.Sleep() simply
> // simulates a consumer that takes variable
> // time to do some work. An actual consumer
> // would not include that.
> Thread.Sleep(rng.Next(2000));
> Console.WriteLine("\t\t\t\tConsumed {0}", i.ToString());
> };
>
> // Call this multiple times for multiple consumers,
> // or just once for only a single consumer. Creating
> // multiple consumers can be very helpful if the data
> // for each queue task is 100% isolated, without
> // shared resources between tasks. Otherwise, be sure
> // to mind the synchronization, and the use of resources
> // to minimize contention (e.g. don't create more
> // consumers for uploading files than you have bandwidth
> // to support).
> queue.StartConsumer();
> queue.StartConsumer();
>
> for (int i = 0; i < 10; i++)
> {
> Console.WriteLine("Producing {0}", i);
> queue.Produce(i);
> Thread.Sleep(rng.Next(1000));
> }
> }
>
> Console.ReadLine();
> }
> }
>
> public class ProducerConsumer<T> : IDisposable
> {
> readonly object listLock = new object();
> readonly Queue<T> queue = new Queue<T>();
> bool fTerminate;
>
> public event Action<T> Consume = delegate { };
>
> public void StartConsumer()
> {
> new Thread(_ConsumerThread).Start();
> }
>
> public void StopAllConsumers()
> {
> lock (listLock)
> {
> fTerminate = true;
> Monitor.PulseAll(listLock);
> }
> }
>
> public void Produce(T t)
> {
> lock (listLock)
> {
> if (fTerminate)
> {
> throw new InvalidOperationException(
> "Consumers are shutting down");
> }
>
> queue.Enqueue(t);
>
> Monitor.Pulse(listLock);
> }
> }
>
> public void _ConsumerThread()
> {
> while (true)
> {
> T t = default(T);
>
> lock (listLock)
> {
> while (queue.Count == 0)
> {
> if (fTerminate)
> {
> return;
> }
> Monitor.Wait(listLock);
> }
>
> t = queue.Dequeue();
> }
>
> Consume(t);
> }
> }
>
> #region IDisposable Members
>
> public void Dispose()
> {
> Dispose(true);
> GC.SuppressFinalize(this);
> }
>
> protected virtual void Dispose(bool fDisposing)
> {
> if (fDisposing)
> {
> // dispose any managed resources here.
> // for now, none are present in this
> // implementation
> }
>
> StopAllConsumers();
> }
>
> ~ProducerConsumer()
> {
> Dispose(false);
> }
>
> #endregion
> }


 |  Next  |  Last
Pages: 1 2
Prev: DSOFramer Successor
Next: issue when using functions (having CString& type parameter) from C++ dll in to C#