The "client" being the remote computer, I assume? When they call
ProcessMessage, does each client get its own thread for the call? The
comment in your code suggests it does, but I want to verify, since I'm not
that familiar with the web service architecture.
This is my first venture into WCF, but yes this is how I understand it
to work. Yes, clients are remote computers. I'm hosting a web service
that is accessible over HTTP in a process that is run as a windows
service. (setting that up is all part of the WCF framework) I'm
assuming the WCF has some sort of thread pool that services all of the
incoming requests from clients. Ultimately all of these threads call
into ProcessMessage.
I don't understand your parenthetical comment. Are you using UDP? Or is
that one socket connected to some remote endpoint that has nothing to do
with each client? Or do you actually have "a single socket" for each
client, and dedicated threads executing ProcessRequestMessage() and
ProcessResponseMessage() for each client?
I'm using TCP. The one socket that is used connects to a back end
server that only my application has direct access to. Unfortunately it
is a requirement that only one socket can be open so all client
requests are serialized thorough this single socket. It would be a lot
easier to be able to just create a new socket for each client but that
is not permitted. ProcessRequestMessage and ProcessResponseMessage are
each running just a single instance on their own thread and are
responsible for sending and receiving every client message. So, no,
there is not an instance of each running for each client.
And where does WCF come into all this? I would have thought if you're
using WCF, you'd use it for everything.
My application acts as the middle layer between clients and the back
end server. WCF is used just to make it easy to expose the application
I'm writing to clients over the network via web services.
This boolean property is the source of at least two issues I see in your
code, neither really related to your original question:
-- The boolean needs to be "volatile". Otherwise, one thread could
set it and the other thread could wind up never actually seeing the value
set
Yea, I've just recently read up on volatile so I will make sure I use
it if I choose to implement it this way.
-- You are polling, which is almost always the wrong approach. The
big hint: you're calling Thread.Sleep(), but you don't have any specific
thing that determines how long you're sleeping for (i.e. you just
arbitrarily choose 10 ms). Any time you find yourself writing code that
calls Thread.Sleep() with an arbitrary value (as opposed to "I know FOR
SURE that in N milliseconds, something interesting will happen), it's a
pretty good bet you're going about things the wrong way.
In this particular case, I think replacing the boolean value with an
AutoResetEvent or ManualResetEvent, or just an object with which you use
the Monitor class (see Monitor.Wait() and Monitor.Pulse()), would be much
better. Then the thread simply blocks indefinitely, until you
specifically wake it up elsewhere.
Yes, I definitely understand that sleeping for an arbitrary amount of
time is not a good way to do this. I wasn't sure what mechanism to use
so I'll look into using AutoResetEvent.
See above. At the very least, you need to make that boolean "volatile"..
Even better, replace it with a different, superior synchronization
mechanism.
You will also probably want to make other fields "volatile", since they
are set in one thread and read in another. The alternative would be to
"lock()" around accesses, but it looks like in this case that's overkill,
since you know the threads aren't actually accessing the data at the same
time. You just need to keep the data up-to-date, not protect it from
multiple threads writing at the same time.
Ok that makes sense.
The biggest problem with the code you provided is that it's not a
concise-but-complete code sample. Of particular concern is that you use
variables for which you have not shown the declaration (e.g.
"messageGuid", "messageID", "transmissionDateAndTime", etc.) and so it's
not possible to comment on whether they are used in a thread-safe way or
not (even with the declarations, without more details about how the
NetworkManager class and its ProcessMessage() method is used, one still
couldn't comment successfully on that, but both are important pieces of
information).
Other issues I see (most of which, again, don't really relate to your
original question but are things I think are important enough to mention):
-- You use a different variable as the key when adding things to the
hash table than the one you use to enqueue the ID, and thus later map back
to the hash table. In this case, it happens the two variables have the
same value, but the code is fragile. It would be much better to make sure
you just use the same variable, rather than relying on the two variables
continuing to have the same value.
-- You have two thread methods that have infinite loops without any
way to break out of them. This suggests you are either aborting the
threads explicitly, or making them background threads and just letting the
framework abort them when the process exits. Either way, it's a very bad
idea. You should provide a way to signal to the threads to exit, so that
you can ensure that they always do so cleanly and in a consistent state.
-- You call Queue.Dequeue() without any check to make sure there's
actually anything in the queue, nor with any sort of synchronization that
would ensure that the thread remains blocked until there's something in
the queue. How your code can even work in this state, I'm not sure.
Perhaps it's related to some of the code you left out. Or maybe you just
have a really serious bug.
Hah, yea, sorry I included just a fraction of the code and actually do
have everything you mentioned above in order to try and keep it
concise and focus on my original question, but I appreciate all of the
comments cause I'm going to make some improvements in other areas.
Now, all that said, as near as I can tell from what code you did post,
your use of the Hashtable class is fine. You've synchronized the code
that writes to the Hashtable and there's no need to synchronize the code
that reads from it, so accesses to the Hashtable should remain consistent..
Given the need to synchronize the writes, I remain unconvinced that
there's really any point in using Hashtable. Seems to me you might as
well use Dictionary<TKey, TValue> and synchronize the reads and writes
both. But then, I would have explicitly synchronized access to an
instance of Queue<T> instead of using the Queue wrapper you get from the
Queue.Synchronized property (at least, I assume that's part of the code
you left out). So, obviously there's a bit of a difference of opinion
with respect to style there.
Pete
So after all of that I am actually leaning this way now. Explicitly
implementing all of the locking and the type safety of
Dictionary<TKey, TValue> does seem a lot more robust. I saw a previous
post of yours where you mentioned using Dictionary<TKey, TValue> with
ReaderWriterLockSlim so I'm looking into that.
Thanks.