|
Hey, this is for c#.
Get Visual Studio .NET
|
|
|
|
|
I hate to be negative, but people use this site to find example code... I don't know where to start with this. It scares me to think that people might look at this and base production code on it. (Or worse, that this might have come from production code.) Here are some things that are wrong:
In the WaitingForClient() method, the connectId code is just...wierd. It's not even guaranteed to be thread-safe, although the only scenario I can think of that will break it off the top of my head requires 10000 simultaneous threads to be running, so it's probably not that bad. (Although it's completely unclear as to why it is using that strange algorithm.)
However, the place where the value of connectId is then used - the ReadSocket method on another thread - is a hostage to fortune - what if the WaitingForClient() method manages to accept a second incoming request before the second one gets as far as reading connectId? (Also, ReadSocket implies connectId is a 'long' - why? Its value is constrained to be 10000 or lower, so having a 64 bit value is just pointless. And copying one long into another like that is not guaranteed to be thread-safe on 32 bit systems.)
The server fires up one thread per connection. This is widely recognised to be an unscaleable solution.
The ReadSocket method updates a ListView directly. This is illegal - you are not supposed to call methods on a Control on any thread other than the one that created it. These calls should be done via the Control.Invoke method or Control.BeginInvoke. (Interestingly LoadThread gets this right when updating the ListView.)
The CloseTheThread mysteriously aborts the thread. This is completely pointless, since the thread was about return out of its starting function anyway, at which point the system would have cleaned it up. Although if the race condition discussed above happens and it gets the wrong value for realId, then it will just abort some other thread... LoadThread also apparently aborts itself instead of just exiting cleanly (although it's hard to be sure - we don't get to see where fThd is set).
CloseTheThread also modifies the socketHolder collection without any synchronisation. This is not thread-safe - if the WaitingForClient code happens to receive a new incoming connection at the same time as another connection shuts down, this will go wrong, possibly corrupting the contents of socketHolder.
LoadThread uses asynchronous invocation - it calls Control.BeginInvoke, and then mysteriously calls JobDone.WaitOne() to wait for this asynchronous work to complete. Not only do you not need to go via any kind of extra object to do this (it was unnecessary to have a JobDone object - the call object returned from BeginInvoke will let you wait for the call to complete - just pass it back into EndInvoke), but as far as I can tell there was no need to use async invocation at all: the only thing this thread does in between starting the call and waiting for it to finish is to sleep for 200ms! If it's important that this loop waits 200ms each time round, then that's fine (although I'm not sure that it is importnat - this has more of a feeling of a hack to it), but that's not reason not to just call Control.Invoke - if you want to do a synchronous call, then use the facility supplied! It's much simpler than async!
The SendDataToAllClient method is fine so long as we ignore the title of the article. If this is supposed to be supporting real-time updates, then this is a particularly bad idea - these Send calls could block. So all it takes is one dead client, and everyone else is stuffed. (Although even fixing that, I would hesitate to use the term "Real Time" to describe anything in this program. I would not trust this kind of code to manage my car engine's ignition timing...)
UpdateListView calls ToString on the symbol every time round the loop where it tries to find a particular entry in the listview.
--
Ian Griffiths
DevelopMentor
|
|
|
|
|
Ian Griffiths wrote:
I hate to be negative, but people use this site to find example code... I don't know where to start with this. It scares me to think that people might look at this and base production code on it. (Or worse, that this might have come from production code.) Here are some things that are wrong:
-- The negative is welcome, but before issue your negative, your
-- negative should be correct, otherwise it is another “terrible”.
In the WaitingForClient() method, the connectId code is just...wierd. It's not even guaranteed to be thread-safe, although the only scenario I can think of that will break it off the top of my head requires 10000 simultaneous threads to be running, so it's probably not that bad. (Although it's completely unclear as to why it is using that strange algorithm.)
-- No. WaitingForClient() is thread safe. It is controlled by the
-- while loop and the tcpLsn.AcceptSocket(). The only one client
-- can be accepted at same time. I use Interlocked.Increment(ref
-- connectId) to increase it, This may don’t need to lock it. The
-- connectId is just a key of the Hashtable. The 10000 is nothing
-- to do with the thread number. connectId defines Max. value of
-- the Hashtable key. I have another variable MaxConnected that
-- control the Max. Connection. Please understand it first!
However, the place where the value of connectId is then used - the ReadSocket method on another thread - is a hostage to fortune - what if the WaitingForClient() method manages to accept a second incoming request before the second one gets as far as reading connectId? (Also, ReadSocket implies connectId is a 'long' - why? Its value is constrained to be 10000 or lower, so having a 64 bit value is just pointless. And copying one long into another like that is not guaranteed to be thread-safe on 32 bit systems.)
-- The connectId is increased by each connection. How long is the
-- server running and how many connections will have during the
-- server is up? The connectId will be a huge number eventually.
-- Even it defined as long, it can’t guarantee it is fine. So I
-- set it to be 10000, or any big number. When the connectId
-- reach 10000, then try to start it from 1 again. You can find a
-- value that is not used by previous close thread.
The server fires up one thread per connection. This is widely recognised to be an unscaleable solution.
-- It depends on how does the server is used and how many client
-- does it have. It has its advantage.
The ReadSocket method updates a ListView directly. This is illegal - you are not supposed to call methods on a Control on any thread other than the one that created it. These calls should be done via the Control.Invoke method or Control.BeginInvoke. (Interestingly LoadThread gets this right when updating the ListView.)
-- You are wrong completely and should learn something from here.
-- This is the best solution to update the control from the
-- background thread! Otherwise you should get the instances of
-- the form and the instance of the control from the background
-- thread. It is not that easy! Please read the example in how to
-- do ... that demonstrates how to create a background thread
-- that uses a MethodInvoker to update a ProgressBar control at
-- regular intervals, which come from Microsoft within the Beta2
-- DVD.
The CloseTheThread mysteriously aborts the thread. This is completely pointless, since the thread was about return out of its starting function anyway, at which point the system would have cleaned it up. Although if the race condition discussed above happens and it gets the wrong value for realId, then it will just abort some other thread... LoadThread also apparently aborts itself instead of just exiting cleanly (although it's hard to be sure - we don't get to see where fThd is set).
-- execute the Abort thread function when you don’t need the
-- thread. It is better than you depend on system to do it for
-- you. If the system does not do it for you, what happen …?
CloseTheThread also modifies the socketHolder collection without any synchronisation. This is not thread-safe - if the WaitingForClient code happens to receive a new incoming connection at the same time as another connection shuts down, this will go wrong, possibly corrupting the contents of socketHolder.
-- You are right at here.
LoadThread uses asynchronous invocation - it calls Control.BeginInvoke, and then mysteriously calls JobDone.WaitOne() to wait for this asynchronous work to complete. Not only do you not need to go via any kind of extra object to do this (it was unnecessary to have a JobDone object - the call object returned from BeginInvoke will let you wait for the call to complete - just pass it back into EndInvoke), but as far as I can tell there was no need to use async invocation at all: the only thing this thread does in between starting the call and waiting for it to finish is to sleep for 200ms! If it's important that this loop waits 200ms each time round, then that's fine (although I'm not sure that it is importnat - this has more of a feeling of a hack to it), but that's not reason not to just call Control.Invoke - if you want to do a synchronous call, then use the facility supplied! It's much simpler than async!
-- you are wrong at here. The BeginInvoke Executes the given
-- delegate on the thread that owns this Control's underlying
-- window handle. The delegate is called asynchronously, but this
-- method returns immediately. it doesn't wait the called
-- function to finished. You have to use JobDone.WaitOne() at
-- here to receive the signal from the end of the called
-- function, then read another data from file.
The SendDataToAllClient method is fine so long as we ignore the title of the article. If this is supposed to be supporting real-time updates, then this is a particularly bad idea - these Send calls could block. So all it takes is one dead client, and everyone else is stuffed. (Although even fixing that, I would hesitate to use the term "Real Time" to describe anything in this program. I would not trust this kind of code to manage my car engine's ignition timing...)
-- I can fix it easily by using try – catch to handle exception,
-- when you got the exception remove all the connected from all
-- the Hashtable.
|
|
|
|
|
I agree that on just one of the points I was wrong, but on all the others I stand by my opinion that this is a bad example. It has several programming errors, some unnecessarily complex idioms, and, crucially, fails to meet the basic requirements for an application; it functions as a toy application but it is fundamentally flawed as production code.
I concede that I failed to take into account that everything is funnelled through accept, so in fact the WaitingForClient() method doesn't *need* to be thread-safe. (However your claim that it is thread-safe remains untrue; it just doesn't need to be thread-safe when it comes to multiple incoming requests, so that particular issue isn't a problem.)
But I stand by my claim that the use of interlocked increment in this function is pointless.
--I use Interlocked.Increment(ref connectId) to increase it
Why? What's wrong with ++connectId; ? (Actually a better way of phrasing that would be: what problems with ++connectId; does Interlocked.Increment solve here? There are actually problems with ++connectId;, but using Interlocked.Increment doesn't solve them in this particular case.) Not only is Interlocked.Increment unnecessary here, it's positively misleading, since it implies that this code is trying to be thread-safe.
If the reason that you're using Interlocked.Increment here is because you are *reading* from the connectId field in another thread, I'm afraid this doesn't cut it. Just because thread A uses Interlocked.Increment, this doesn't alter the fact that if thread B reads the field, that read will be non-atomic: on a 32 bit processor, reading connectId will require 2 bus cycles, and the operation will be non-atomic. If you don't believe me go and read the part of the CLR specs that talk about which memory accesses are atomic and which aren't. The first bus cycle could occur before the Interlocked.Increment, the second could occur after the Interlocked.Increment. The only thing that Interlocked.Increment guarantees is that the read can't occur half way through the increment; it doesn't stop the opposite though - the Increment *can* happen half way through the read. Both the increment AND the read would need to be atomic for your code to be correct. (AFAIK there is no way to do an Interlocked.Read - if you could do an interlocked read on a long, you would be able to avoid this problem.)
If connectId were an int, not a long, it would be different. Reading from an int *is* guaranteed to be atomic. So that would fix the problem your code has wherebye the threads you kick off could actually read a bad value by getting rid of the potential for a read from a long being split by the increment. And since you are restricting the value of connectId to always be less than 10,000 I can see absolutely no point in making it a long. (You are aware that long is a 64 bit type aren't you?) Make it an int, and this gets rid of this particular set of problems.
However, that's not all... You completely fail to address the points in the next section:
>>However, the place where the value of connectId is then used
>>- the ReadSocket method on another thread - is a hostage to
>>fortune - what if the WaitingForClient() method manages to
>>accept a second incoming request before the second one gets
>>as far as reading connectId? (Also, ReadSocket implies
>> connectId is a 'long' - why? Its value is constrained to be
>> 10000 or lower, so having a 64 bit value is just pointless.
>> And copying one long into another like that is not guaranteed
>> to be thread-safe on 32 bit systems.)
-- The connectId is increased by each connection. How long is the
-- server running and how many connections will have during the
-- server is up? The connectId will be a huge number eventually.
-- Even it defined as long, it can’t guarantee it is fine. So I
-- set it to be 10000, or any big number. When the connectId
-- reach 10000, then try to start it from 1 again. You can find a
-- value that is not used by previous close thread.
This doesn't address either of the issues I raised, so I'll state them again:
(1) If the main thread manages to accept an incoming connection, launch a new thread, and then accept ANOTHER incoming connection before the 1st thread that it launched gets as far as reading the connectId, the connectId will already have been incremented a 2nd time. (This is possible despite the serialization that going through accept enforces: there is nothing in your main code that will wait until a newly-launched thread has actually read the value of connectId.) In this case both of the threads will read the same value from connectId. You will now have two threads that think they have the same ID. This is a bug.
(2) Why have you made connectId a long? long is a 64 bit type. You don't need a 64 bit type to hold a number that never goes over 10,000.
>> The server fires up one thread per connection. This is
>> widely recognised to be an unscaleable solution.
-- It depends on how does the server is used and how many client
-- does it have. It has its advantage.
Document this fact then. You don't make it clear that this code is only appropriate for lightweight usage, and that it will fall over under any kind of load. The system thread pool limits the maximum number of threads that it creates to 25 on a single processor system. It does this for a very good reason. Your system is OK for very small numbers of clients, but for 50 clients or more, it's not good. For 100s of clients it will have big performance problems.
The advantage your approach has is simplicity of implementation. The disadvantage is that this is no good for large (or even moderate) numbers of clients. At least mention this in your article, or people will think this is a good technique. I've seen servers that have floundered using this technique for fewer than 50 clients!
>>The ReadSocket method updates a ListView directly. This
>>is illegal - you are not supposed to call methods on a
>>Control on any thread other than the one that created it.
>>These calls should be done via the Control.Invoke method
>>or Control.BeginInvoke. Interestingly LoadThread gets
>>this right when updating the ListView.)
-- You are wrong completely and should learn something from here.
-- This is the best solution to update the control from the
-- background thread!
Well I can back my statement up with authoritative sources: I quote from Microsoft's documentation:
"Windows Forms uses the single-threaded apartment (STA) model because Windows Forms is based on native Win32 windows that are inherently apartment threaded. The STA model implies that a window can be created on any thread, but it cannot switch threads once created, and all function calls to it must occur on its creation thread."
and
"The STA model requires that any methods on a control that need to be called from outside the control's creation thread must be marshaled to (executed on) the control's creation thread. "
I also refer you to Mark Boulter's comment on this subject. Mark Boulter works for Microsoft. In fact he works for the part of Microsoft responsible for producing the Windows Forms framework. When someone did exactly what you suggest (updating a control from a background thread) here is what he said:
"DON'T DO THIS. YOU GOT LUCKY. YOU MUST USE Control.Invoke etc."
The original message is here. For a more full explanation see this message he wrote here, and to quote a small section from it, he says:
"if you are using multiple threads of execution you must be careful to marshal any calls to Win Forms controls back to the creating thread"
So the very thing you recommend (updating controls from background threads) is explicitly not allowed, according to Mark Boulter. Since he is a developer working for the part of Microsoft who produced this technology, I would very much like to know why it is that you claim to know more than him about how to use Windows Forms Controls.
--Otherwise you should get the instances of
-- the form and the instance of the control from the background
-- thread. It is not that easy! Please read the example in how to
-- do ... that demonstrates how to create a background thread
-- that uses a MethodInvoker to update a ProgressBar control at
-- regular intervals, which come from Microsoft within the Beta2
-- DVD.
I'm not sure from this whether you're having trouble with expressing the ideas in English (I'm guessing English is not your first language) or whether you genuinely don't understand the issues.
First of all this is *not* optional. The fact that it is inconvenient doesn't alter the fact that you have to do it. It's not just me who says you have to do it - the documentation, and Mark Boulter say that too.
Secondly, it's unclear what you mean by getting "the instance of the control from the background thread." This is not really the issue - there is only one instance of a given control, it's easy enough to get. The important thing is that you have to marshal calls onto the right thread if you want to use if from a background thread.
I'm well aware of the example in the documentation. It's really not *that* hard, and the whole point is that it's NOT OPTIONAL! If you find it difficult, then I'm very sorry to hear that, but that doesn't excuse you. You still have to do it. You are absolutely required to use Control.Invoke (or BeginInvoke). (You don't actually have to use MethodInvoker by the way - you can use any delegate.) It's the use of the [Begin]Invoke method that's significant.
Your code doesn't use this technique. It should. It's not just me who says it, but the documentation and the guys at Microsoft who design this stuff. So don't go telling me that I'm the one who could learn here. 
>>The CloseTheThread mysteriously aborts the thread.
>> This is completely pointless, since the thread was
>> about return out of its starting function anyway,
>> at which point the system would have cleaned it up.
-- execute the Abort thread function when you don’t need the
-- thread. It is better than you depend on system to do it for
-- you. If the system does not do it for you, what happen …?
If the system does not do it for you then there is a bug in the system. So you are writing code that presumes that at some point Microsoft will release a version of the CLR which has a bug wherebye threads don't terminate when they should. Don't you think that this is a little paranoid?
As soon as the main function in a thread exits, the thread will terminate. It's not like it needs to hang around and wait for garbage collection. Aborting the thread will, if anything, slow things down, since forcibly aborting a thread is more expensive than letting it terminate naturally.
Normally you would only abort a thread from another thread. I.e. the purpose of the Abort method is to allow one thread to kill off another one. There is rarely any need for a thread to kill itself off like this. (In your case there is definitely no need, since it is easy for the threads to simply exit.)
>>LoadThread uses asynchronous invocation - it calls
>>Control.BeginInvoke, and then mysteriously calls
>>JobDone.WaitOne() to wait for this asynchronous
>>work to complete [...]
You have completely missed the point I was making here in your answer:
-- you are wrong at here. The BeginInvoke Executes the given
-- delegate on the thread that owns this Control's underlying
-- window handle. The delegate is called asynchronously,
I know. That's what I said. That's what I meant when I said that "LoadThread uses asynchronous invocation - it calls Control.BeginInvoke". I have no idea why you think that your reply contradicts what I said, so I'm not sure what you're disagreeing with.
My point is that having used asynchronous invocation, you then have a load of extremely complex code that recreates the exact same semantics as you would have had if you had used synchronous invocation in the first place. So my question is: why didn't you use synchronous invocation in the first place?
-- but this method returns immediately.
Well...yes. That's exactly what asynchronous invocation is about. If it waited until the method returned then it would be synchronous invocation. (The fact that the method runs on a different thread is orthogonal to this. A call can be synchronous despite crossing threads - that's precisely what Control.Invoke does!)
-- it doesn't wait the called function to finished.
-- You have to use JobDone.WaitOne() at
-- here to receive the signal from the end of the called
-- function,
Exactly! That's precisely my point. (Again, I'm guessing that this is a language barrier problem - you seem to be in violent agreement with me, and yet you don't agree that the code is wrong.) If you wanted to wait for the function to finish, why didn't you use the method that DOES THAT FOR YOU?!!! (Control.Invoke)
Just in case you've misunderstood the documentation, here's what Control.Invoke does: it runs the method on the Control's thread, but it doesn't return until that method has completed. These are exactly the semantics you have recreated here. But the advantage of using Control.Invoke is that it's just one line of code, rather than the convoluted solution you have presented.
>> The SendDataToAllClient method is fine so
>> long as we ignore the title of the article.
>> If this is supposed to be supporting real-time
>> updates, then this is a particularly bad idea -
>> these Send calls could block. So all it takes
>> is one dead client, and everyone else is stuffed.
-- I can fix it easily by using try – catch to handle exception,
-- when you got the exception remove all the connected from all
-- the Hashtable.
That would be better, but it's only a partial solution. It takes a long time for the TCP/IP layer to detect that a connection is dead - far too long for the demanding requirements of real time stock price information. Worse than that, if you have a connection that is flakey but not dead (such as you get on the internet all the time - when you get heavy but not complete packet loss, connections grind to a near-halt but never actually die), then send operations can block for minutes at a time. Worse than that, it's possible for a client process to be locked up but the network to be healthy, in which case the call will block indefinitely, and there will be no errors. Ever.
So even with error handling built in, this code is entirely inadequate for the problem at hand. There is absolutely no sense in which it is real time - it is limited by the slowness of the slowest client, and there is no bound on how slow that might be. The only way to deal with this is to use a multi-threaded send of some kind. I would fire up a watchdog thread which checks to make sure the sending thread hasn't blocked for more than, say, a second, and to spin up a new thread if it has. Or it might be possible to build a satisfactory solution around the system thread pool. A third approach might be to use non-blocking IO, which would probably allow the most efficient solution, but would be the most complex approach.
I remain convinced that this is an example of how not to write such an application. Sorry.
--
Ian Griffiths
DevelopMentor
|
|
|
|
|
Hi Ian,
Ian write ...
However, that's not all... You completely fail to address the points in the next section:
>>However, the place where the value of connectId is then used
>>- the ReadSocket method on another thread - is a hostage to
>>fortune - what if the WaitingForClient() method manages to
>>accept a second incoming request before the second one gets
>>as far as reading connectId? (Also, ReadSocket implies
>> connectId is a 'long' - why? Its value is constrained to be
>> 10000 or lower, so having a 64 bit value is just pointless.
>> And copying one long into another like that is not guaranteed
>> to be thread-safe on 32 bit systems.)
-- Beacause WaitingClient using while-loop to handle the new
-- connection, After accepted new connection
-- and processed, then can accept another new coming connection.
-- The ReadSocket thread only using conectId when it started.
-- After the thread is started, then WaitingClient can start to
-- accept new connection. The satuation what you concern will be
-- not happened.
>>The ReadSocket method updates a ListView directly. This
>>is illegal - you are not supposed to call methods on a
>>Control on any thread other than the one that created it.
>>These calls should be done via the Control.Invoke method
>>or Control.BeginInvoke. Interestingly LoadThread gets
>>this right when updating the ListView.)
-- You are wrong completely and should learn something from here.
-- This is the best solution to update the control from the
-- background thread!
Well I can back my statement up with authoritative sources: I quote from Microsoft's documentation:
So the very thing you recommend (updating controls from background threads) is explicitly not allowed, according to Mark Boulter. Since he is a developer working for the part of Microsoft who produced this technology, I would very much like to know why it is that you claim to know more than him about how to use Windows Forms Controls.
....
-- Please goto How Do I ... Common tasks, Windows Forms: and
-- Manipulate a control from a background thread?
-- The article: Making procedure calls across thread boundaries
-- and sample code that "demonstrates how to create a background
-- thread that uses a MethodInvoker to update a ProgressBar
-- control at regular intervals:"
-- This is not some personal opinion. It microsoft published code
-- with the Beta 2 DVD. That represents Microsoft. It is created
-- by Microsoft expert. You may think it is terrible sample.
-- Publish your idea on it. Let me know what do you think about
-- the sample. I use the same way that the sample did. I use
-- asynchronous invocation to make everything easy. Why do you
-- force me to use synchronous call that you should get the
-- instance of the form and instance of the listview control from
-- background thread that is extral work when you use
-- Control.Invoke.
So even with error handling built in, this code is entirely inadequate for the problem at hand. There is absolutely no sense in which it is real time - it is limited by the slowness of the slowest client, and there is no bound on how slow that might be. The only way to deal with this is to use a multi-threaded send of some kind. I would fire up a watchdog thread which checks to make sure the sending thread hasn't blocked for more than, say, a second, and to spin up a new thread if it has. Or it might be possible to build a satisfactory solution around the system thread pool. A third approach might be to use non-blocking IO, which would probably allow the most efficient solution, but would be the most complex approach.
-- I like this comments. But This program just a small demo, you
-- can say it is a toy. You can't expect it to solve all the
-- problems in the TCP/IP world.
-- Thanks any way. I holp reader can learn some from these.
Jibin Pan
|
|
|
|
|
-- Beacause WaitingClient using while-loop to handle the new
-- connection, After accepted new connection
-- and processed, then can accept another new coming connection.
-- The ReadSocket thread only using conectId when it started.
-- After the thread is started, then WaitingClient can start to
-- accept new connection. The satuation what you concern will be
-- not happened.
Here is a sequence of events that will cause exactly the problem I describe to occur:
(1) A client connection request comes in
(2) In WaitingForClient, tcpLsn.AcceptSocket returns.
(3) WaitingForClient increments connectId, to (say) 2
(4) Another client connection request comes in. (This won't be handled yet, because - it will only get handled when the main thread loops back round. However, the request will sit in the OS's queue.)
(5) WaitingForClient checks ID 2 is not in use right now
(6) WaitingForClient calls Thread.Start to run the new thread
(7) The OS decides that although it could start running the new thread, it realises that the main thread hasn't used up its current quanta and so it allows the main thread to carry on running.
(8) The main thread loops round, calls tcpLsn.AcceptSocket which returns immediately because of the second client connection request that came in earlier.
(9) The main thread increments connectId to 3.
(10) The main thread kicks off another thread
(11) The main thread loops round again, and this time it blocks in AcceptSocket
(12) The OS decides to give the next thread a go. It starts up, reads connectId and gets the value 3. So it sets its realId value to 3.
(13) The OS scheduler switches to the other thread that got created. It starts up, reads connectId and also gets the value 3.
So there are now two threads running, both of which think their ID is 3. Surely this is a bug?
Do you agree with this analysis? As far as I can tell, this is not only possible, it's positively likely to happen. It's not unusual for a single listening socket to have multiple connection requests queue up faster than they are handled. This is likely to happen when everyone connects to your server first thing in the morning. And the scheduler will very likely behave in the manner I describe - it tries to minimise the number of thread switches, so the main thread will almost certainly carry on running after it has called Thread.Start, and the threads thus launched won't actually begin their work until the main thread runs out of things to do.
There are variations on the theme. If everything happens the same as before up to 8, consider the following alternative sequence after that point:
(9) The OS switches to the other thread.
(10) The other thread starts to read connectId. Remember that reading from a long is not an atomic operation.
(11) The OS decides to switch back to the main thread. The main thread calls Interlocked.Increment to increment connectId. This will succeed - it has no way of knowing that there was another thread trying to read from connectId at the time.
(12) The OS switches back to the other thread, which carries on doing its read.
So the launched thread's value of realId will be corrupt. It has been interrupted by a thread switch. (That can definitely happen - read the CLR spec.) A 64 bit read operation which has been split in this way has an undefined value, so realId could be absolutely anything.
In practice this second scenario is not going to happen very often. But that's no excuse - threading hazards that don't happen often still happen, they just happen once every 6 months or so. In practice I would be surprised if it ever caused a real problem, simply because your value never goes above 10,000 so the top half of the long is always 0, so on current architectures you will happen to get the right value anyway. So you are relying on undocumented behaviour. But what I still don't understand is why on earth you chose to make this a long! Just make it an int, and this particular problem goes away.
However, the other problem - the problem of 2 different threads both getting the same ID won't be solved by changing to an int. That's a more fundamental problem with your design, and require more radical changes.
-- Please goto How Do I ... Common tasks, Windows Forms: and
-- Manipulate a control from a background thread?
-- The article: Making procedure calls across thread boundaries
-- and sample code that "demonstrates how to create a background
-- thread that uses a MethodInvoker to update a ProgressBar
-- control at regular intervals:"
I think we're talking about different parts of your code here. You're talking about StartReceive, where it passes a MethodInvoker to this.BeginInvoke, yes? That code is correct, although I think it would be better to call this.Invoke. (That will still marshal the call to the correct thread, as required. It will also block until UpdateListView has finished, which is what your comments say you want.)
That's not the code that I'm talking about. I'm talking about the code in ReadSocket.
In ReadSocket, you have the following lines of code:
this.listView2.Items.Add(newItem);
ind=this.listView2.Items.IndexOf(newItem);
and also
this.listView2.Items[ind].ImageIndex=1;
The ReadSocket method is not running on the UI thread. The reason I say that is that you also have this line of code:
Thread td = new Thread(new ThreadStart(ReadSocket));
So, the ReadSocket method doesn't run on the main thread, but it does call methods and use properties on listView2. This is wrong. You should be using this.Invoke or this.BeginInvoke here just like you do in your StartReceive method.
So, I wasn't complaining about StartReceive, or UpdateListView. These use the correct technique. I was complaining about the ReadSocket method. This is why I said in my original message "The ReadSocket method updates a ListView directly. This is illegal"
-- I like this comments. But This program just a small demo, you
-- can say it is a toy. You can't expect it to solve all the
-- problems in the TCP/IP world.
No, but I think it's somewhat misleading. You have given this article the title "Real Time TCP/IP using C#", and your example is all based around stock prices. There is nothing about this code that can be described as "Real Time" - it makes no guarantees about timing as it can be brought down by a single client. It is very definitely not a sound basis for a real time stock price update system.
So not only does it not solve all the problems in the TCP/IP world, I don't believe it even solves the problem that the title suggests it solves.
--
Ian Griffiths
DevelopMentor
|
|
|
|
|
-- I have to make some claim again. You didn't understand the
-- codes and your comments will mislead the reader.
For following comments:
>>The CloseTheThread mysteriously aborts the thread.
>> This is completely pointless, since the thread was
>> about return out of its starting function anyway,
>> at which point the system would have cleaned it up.
---- execute the Abort thread function when you don’t need the
---- thread. It is better than you depend on system to do it for
---- you. If the system does not do it for you, what happen …?
If the system does not do it for you then there is a bug in the system. So you are writing code that presumes that at some point Microsoft will release a version of the CLR which has a bug wherebye threads don't terminate when they should. Don't you think that this is a little paranoid?
As soon as the main function in a thread exits, the thread will terminate. It's not like it needs to hang around and wait for garbage collection. Aborting the thread will, if anything, slow things down, since forcibly aborting a thread is more expensive than letting it terminate naturally.
Normally you would only abort a thread from another thread. I.e. the purpose of the Abort method is to allow one thread to kill off another one. There is rarely any need for a thread to kill itself off like this. (In your case there is definitely no need, since it is easy for the threads to simply exit.)
-- First, I don't like your idea that you don't do the clean job
-- and let the operating system to do it for you. If the system
-- have a bug then your program have the bug. You can expect the
-- system is bug free, otherwise Microsoft don't need release
-- some server packages and create new operating systems.
-- Second, ReadSocket() check the client information to
-- determine the thread is needed or not and close the thread
-- when the client is logoff. The operating system can't do it
-- for you. Do you want to keep these threads alive until you
-- close the main thread?
-- I find the link for the update control from background
-- thread on the Web. I have said that "you can learn something
-- from here".
-- is not from me, is from Microsoft. http://samples.gotdotnet.com/QuickStart/aspplus/default.aspx?url=/quickstart/howto/doc/WinForms/WinFormsThreadMarshalling.aspx
-- Please see the code that is same as I did.
|
|
|
|
|
-- First, I don't like your idea that you don't do the clean job
-- and let the operating system to do it for you. If the system
-- have a bug then your program have the bug. You can expect the
-- system is bug free,
Of course no system is bug free, but do you have any reason to suppose that a bug of the nature you describe will emerge? Right now, threads terminate when you return from their ThreadStart function. It would be a pretty serious bug if this stopped working, because it would mean that most programs that launch threads would suddenly start to suffer from serious performance problems as the number of threads in the process stacked up uncontrollably.
So if Microsoft ever *did* manage to introduce such a bug into a future version by mistake, it would break a huge amount of code. Yours is the only program I have ever seen that anticipates such a bug in the runtime, so the vast majority of multithreaded programs would break. This suggests that such a release wouldn't make it through beta testing.
So don't you think you're being a bit paranoid? And even if you do think that it's appropriate to presume that such random and major bugs will appear in future releases, why are you any more confident that aborting the thread will work? If you don't have any faith that Microsoft won't break System.Thread in such a way that it doesn't terminate threads when they exit, why do you have any more faith that System.Thread.Abort won't also be broken in a future release?
Working around known bugs is fine, and standard practice. Putting in workarounds for bugs that have never existed, and which you have no good reason to suppose will *ever* exist is pointless, because if you take that approach to its logical conclusion, you'll never trust the platform to do anything at all!
-- I find the link for the update control from background
-- thread on the Web. I have said that "you can learn something
-- from here".
-- is not from me, is from Microsoft. http://samples.gotdotnet.com/QuickStart/aspplus/default.aspx?url=/quickstart/howto/doc/WinForms/WinFormsThreadMarshalling.aspx
-- Please see the code that is same as I did.
Following this link, the first thing it says is:
"Windows Forms controls can only execute on the thread on which they were created"
This is precisely my point. I'll say it one more time:
The problem is in your ReadSocket function.
The code you have in the ReadSocket function does NOT work in the way recommended by that article.
PLEASE GO AND RE-READ THE CODE IN THE ReadSocket FUNCTION. I know that your code does it correctly in the StartReceive function and the LoadThread function - that code does indeed do the same thing as the article. I'm not complaining about the StartReceive function or the LoadThread function. I'm complaining about the ReadSocket function, which does several things to listView2, despite running on a non-UI thread. The code in the ReadSocket function does NOT follow the recommendations (i.e. it doesn't use Control.Invoke), which is what I'm complaining about.
There is not a single call to Invoke or BeginInvoke inside the ReadSocket method, nor do you use MethodInvoker in there.
I don't know how many more ways I can find to say this. Your code gets it right in two places (StartReceive and LoadThread) and wrong in one place: ReadSocket
As a side issue, I think you have misunderstood why Microsoft's example calls Thread.Sleep(500) after BeginInvoke - it is only doing that to fake up some 'work'. That's in their example because their worker thread doesn't do anything real, so it just sleeps instead. You didn't need to copy that bit across. It doesn't make your code actively wrong as such, but it does mean that it pointlessly wastes some time.
--
Ian Griffiths
DevelopMentor
|
|
|
|
|
I think Ian Griffiths is a total tit... No half decent coder types so much crap if he's such a mentor then where thy code?
Don't need many brain threads on that! Mentor My Arse? Minetor More Like...
The IT bizz is full of Dome Heads Like This Guy Thats Why It Sucks.
Thank 4 your example anyhow.;P ;)    ((           :->  
|
|
|
|
|
IIan
Can you point me to an article of how to write a TCPClient/TCPServer?
|
|
|
|
|
|
High,
i read a message, that you worked on a socket server in C# Forms.
I need to do a socket server which communicates with up to 100 client. they exchange only a few bytes.
What i want is a Form an which i can put the communication in Listviews and store thme into the sqlserver via ado.net. Is there any sample in the www for this or parts of this?
I only find Console.Apps or the sample in codeprojct which may be not save for 24/7 working environment.
Any info ?
Thanks for your help,
christian
|
|
|
|
|
Hi,
listening at 0.0.0.0:8002 means that your program is listening to ANY connected attempt to port 8002 (from RAS, LAN, ...)
If your specify an IP adress then only requests that are received from the specified adapter are accepted. The your output will show the specified ip adress.
Ciao Matthias Mann
|
|
|
|
|
Now, I didn't read your article very carefully, but it seems to me that every writing to socketHolder and threadHolder should be "locked"
I vote pro drink ![Beer | [beer]](https://codeproject.freetls.fastly.net/script/Forums/Images/beer.gif)
|
|
|
|
|
Please add WTcpClient to your zipped file. Thanks.
|
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
