I feel dirty...
-
... I used my first goto today :shudders: to workaround this problem[^] I've been having.
private void BroadcastPacket(string Packet, FSClient client) { List ClientsToRemove = new List(); ASCIIEncoding encoder = new ASCIIEncoding(); byte\[\] PacketBytes = encoder.GetBytes(Packet); lock (ClientLock) { foreach (FSClient CurrentClient in clients) { if (client != CurrentClient) { try { if (CurrentClient.Socket.Connected) { TryAgain: try { CurrentClient.Socket.Send(PacketBytes, PacketBytes.Length, SocketFlags.None); bytesSent += PacketBytes.Length; } catch (SocketException ex) { if (ex.SocketErrorCode == SocketError.WouldBlock) { Console.WriteLine("SocketError.WouldBlock Occured"); Thread.Sleep(300); goto TryAgain; } } } else { ClientsToRemove.Add(CurrentClient); } } catch (Exception ex) { Console.WriteLine("Error @ BroadcastPacket(): " + ex.Message); ClientsToRemove.Add(CurrentClient); } } } } foreach (FSClient CurrentClient in ClientsToRemove) { CloseConnection(CurrentClient); } }I'm going straight to hell :((
-
goto? No problem! On the other hand, those annoyingtry-catchblocks around the bravegotostatement... :rolleyes: Don't be afraid of usinggotos. Klingon developers usegoto-hellhere and there (in fact Klingons useC, the only language of real men...). Strange enough, after40(forty!) years since the (in)famous criticisms to thegotostatement, the MS 'fireblade' language still provides it... :laugh:If the Lord God Almighty had consulted me before embarking upon the Creation, I would have recommended something simpler. -- Alfonso the Wise, 13th Century King of Castile.
This is going on my arrogant assumptions. You may have a superb reason why I'm completely wrong. -- Iain Clarke
[My articles]CPallini wrote:
in fact Klingons use C, the only language of real men...
Mind if I quote that and use it everywhere I go? :laugh:
-
... I used my first goto today :shudders: to workaround this problem[^] I've been having.
private void BroadcastPacket(string Packet, FSClient client) { List ClientsToRemove = new List(); ASCIIEncoding encoder = new ASCIIEncoding(); byte\[\] PacketBytes = encoder.GetBytes(Packet); lock (ClientLock) { foreach (FSClient CurrentClient in clients) { if (client != CurrentClient) { try { if (CurrentClient.Socket.Connected) { TryAgain: try { CurrentClient.Socket.Send(PacketBytes, PacketBytes.Length, SocketFlags.None); bytesSent += PacketBytes.Length; } catch (SocketException ex) { if (ex.SocketErrorCode == SocketError.WouldBlock) { Console.WriteLine("SocketError.WouldBlock Occured"); Thread.Sleep(300); goto TryAgain; } } } else { ClientsToRemove.Add(CurrentClient); } } catch (Exception ex) { Console.WriteLine("Error @ BroadcastPacket(): " + ex.Message); ClientsToRemove.Add(CurrentClient); } } } } foreach (FSClient CurrentClient in ClientsToRemove) { CloseConnection(CurrentClient); } }I'm going straight to hell :((
You could use a boolean flag and a while loop. But I don't see anything wrong with goto really and goto is probably how I'd do it. I sometimes wonder if there's anything bad about using them in catch blocks though, but haven't ever come across or read about any problems associated with it.
-
I'd definitely consider refactoring the code if I were you, but not because of the goto statement. The Thread.Sleep is a bit of a no-no though - have a read of this thread[^] (pardon the pun).
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
Interesting. I've never come across that before. The one place I have used Thread.Sleep is where I have a .net app that is communicating with a horrible nasty old server from circa 1825 via telnet. The nasty old server in question runs an app that is so rubbish it needs messages to arrive slowly so it has time to process them. Hence my implementation of communicating with it is pretty much this:
Connection con = OpenConnection("StupidServer");
SendMessage("Hi");
Thread.Sleep(1000);
SendMessage("Do something");
Thread.Sleep(1000);
SendMessage("Do yet another pointless and stupid thing");
Thread.Sleep(1000);
SendMessage("I don't want to talk to you anymore, you are dull and boring. Goodbye");
Thread.Sleep(1000);
CloseConnection(con);(server name and messages changes to protect identity) I don't really care about exact timings, I just need to make sure my messages don't go too fast or the server will blow up. Rewriting the server to add in ready acknowledgements, or the ability to queue messages or something like that is not an option. All this does happen on a background thread so the GUI isn't being blocked. I think that is a pretty valid use of Thread.Sleep. Anyone disagree? Is there an obvious better way I've missed.
Simon
-
... I used my first goto today :shudders: to workaround this problem[^] I've been having.
private void BroadcastPacket(string Packet, FSClient client) { List ClientsToRemove = new List(); ASCIIEncoding encoder = new ASCIIEncoding(); byte\[\] PacketBytes = encoder.GetBytes(Packet); lock (ClientLock) { foreach (FSClient CurrentClient in clients) { if (client != CurrentClient) { try { if (CurrentClient.Socket.Connected) { TryAgain: try { CurrentClient.Socket.Send(PacketBytes, PacketBytes.Length, SocketFlags.None); bytesSent += PacketBytes.Length; } catch (SocketException ex) { if (ex.SocketErrorCode == SocketError.WouldBlock) { Console.WriteLine("SocketError.WouldBlock Occured"); Thread.Sleep(300); goto TryAgain; } } } else { ClientsToRemove.Add(CurrentClient); } } catch (Exception ex) { Console.WriteLine("Error @ BroadcastPacket(): " + ex.Message); ClientsToRemove.Add(CurrentClient); } } } } foreach (FSClient CurrentClient in ClientsToRemove) { CloseConnection(CurrentClient); } }I'm going straight to hell :((
Here's a none-Goto version:
private static readonly object _syncLock = new object();
private void BroadcastPacket(string Packet, FSClient client)
{
List<FSClient> ClientsToRemove = new List<FSClient>();
byte[] PacketBytes = new ASCIIEncoding().GetBytes(Packet);
bool retry;lock (ClientLock)
{
foreach (FSClient currentClient in clients)
{
try
{
if (client != currentClient)
{
do
{
retry = ProcessPacket(PacketBytes);
} while (retry);
}
else
{
ClientsToRemove.Add(client);
}
}
catch
{
ClientsToRemove.Add(client);
}
}
foreach (FSClient CurrentClient in ClientsToRemove)
{
CloseConnection(CurrentClient);
}
}
}private bool ProcessPacket(byte[] packetBytes)
{
bool retry = false;
try
{
CurrentClient.Socket.Send(packetBytes, packetBytes.Length, SocketFlags.None);
bytesSent += packetBytes.Length;
}
catch (SocketException ex)
{
if (ex.SocketErrorCode == SocketError.WouldBlock)
{
retry = true;
lock (_syncLock)
{
Monitor.Wait(_syncLock, 300);
}
}
}
return retry;
}I typed this in Notepad, so it might have a couple of errors, but it should do the same thing as the goto version.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
-
Interesting. I've never come across that before. The one place I have used Thread.Sleep is where I have a .net app that is communicating with a horrible nasty old server from circa 1825 via telnet. The nasty old server in question runs an app that is so rubbish it needs messages to arrive slowly so it has time to process them. Hence my implementation of communicating with it is pretty much this:
Connection con = OpenConnection("StupidServer");
SendMessage("Hi");
Thread.Sleep(1000);
SendMessage("Do something");
Thread.Sleep(1000);
SendMessage("Do yet another pointless and stupid thing");
Thread.Sleep(1000);
SendMessage("I don't want to talk to you anymore, you are dull and boring. Goodbye");
Thread.Sleep(1000);
CloseConnection(con);(server name and messages changes to protect identity) I don't really care about exact timings, I just need to make sure my messages don't go too fast or the server will blow up. Rewriting the server to add in ready acknowledgements, or the ability to queue messages or something like that is not an option. All this does happen on a background thread so the GUI isn't being blocked. I think that is a pretty valid use of Thread.Sleep. Anyone disagree? Is there an obvious better way I've missed.
Simon
It looks OK in this case. The problem with
Thread.Sleepis that it is not guaranteed to sleep for the specified time. It may take few ticks more than what specified. But since you don't care about exact timings, it should be a valid usage. One alternative method is to useWaitHandleand pass a timeout value toWaitOnemethod.Best wishes, Navaneeth
-
Interesting. I've never come across that before. The one place I have used Thread.Sleep is where I have a .net app that is communicating with a horrible nasty old server from circa 1825 via telnet. The nasty old server in question runs an app that is so rubbish it needs messages to arrive slowly so it has time to process them. Hence my implementation of communicating with it is pretty much this:
Connection con = OpenConnection("StupidServer");
SendMessage("Hi");
Thread.Sleep(1000);
SendMessage("Do something");
Thread.Sleep(1000);
SendMessage("Do yet another pointless and stupid thing");
Thread.Sleep(1000);
SendMessage("I don't want to talk to you anymore, you are dull and boring. Goodbye");
Thread.Sleep(1000);
CloseConnection(con);(server name and messages changes to protect identity) I don't really care about exact timings, I just need to make sure my messages don't go too fast or the server will blow up. Rewriting the server to add in ready acknowledgements, or the ability to queue messages or something like that is not an option. All this does happen on a background thread so the GUI isn't being blocked. I think that is a pretty valid use of Thread.Sleep. Anyone disagree? Is there an obvious better way I've missed.
Simon
If I were coding this thing from scratch I'd use something like a Monitor, and a background thread to handle this. I'd use Monitor.Wait to wait for the child thread to poke the parent to wake it up - something like this:
lock (_syncLock)
{
SendMessage("Hi"); // SendMessage operates on a background thread, and uses Pulse(_syncLock) to
// notify the parent thread that it's finished processing.
Monitor.Wait(_syncLock, 1000);
}Basically, by doing this you can wake the parent up if you finish processing earlier than 1000 milliseconds, or the parent drops out at roughly 1 second. You could always use an Infinite timeout here to wait until the child thread has completed processing.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
-
Here's a none-Goto version:
private static readonly object _syncLock = new object();
private void BroadcastPacket(string Packet, FSClient client)
{
List<FSClient> ClientsToRemove = new List<FSClient>();
byte[] PacketBytes = new ASCIIEncoding().GetBytes(Packet);
bool retry;lock (ClientLock)
{
foreach (FSClient currentClient in clients)
{
try
{
if (client != currentClient)
{
do
{
retry = ProcessPacket(PacketBytes);
} while (retry);
}
else
{
ClientsToRemove.Add(client);
}
}
catch
{
ClientsToRemove.Add(client);
}
}
foreach (FSClient CurrentClient in ClientsToRemove)
{
CloseConnection(CurrentClient);
}
}
}private bool ProcessPacket(byte[] packetBytes)
{
bool retry = false;
try
{
CurrentClient.Socket.Send(packetBytes, packetBytes.Length, SocketFlags.None);
bytesSent += packetBytes.Length;
}
catch (SocketException ex)
{
if (ex.SocketErrorCode == SocketError.WouldBlock)
{
retry = true;
lock (_syncLock)
{
Monitor.Wait(_syncLock, 300);
}
}
}
return retry;
}I typed this in Notepad, so it might have a couple of errors, but it should do the same thing as the goto version.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
For your next exercise, convert the above to use CPS, and use
success,retryandfailurecontinuations. Bonus points if you can implemented it in C# :)xacc.ide
IronScheme - 1.0 RC 1 - out now!
((λ (x) `(,x ',x)) '(λ (x) `(,x ',x))) The Scheme Programming Language – Fourth Edition -
If I were coding this thing from scratch I'd use something like a Monitor, and a background thread to handle this. I'd use Monitor.Wait to wait for the child thread to poke the parent to wake it up - something like this:
lock (_syncLock)
{
SendMessage("Hi"); // SendMessage operates on a background thread, and uses Pulse(_syncLock) to
// notify the parent thread that it's finished processing.
Monitor.Wait(_syncLock, 1000);
}Basically, by doing this you can wake the parent up if you finish processing earlier than 1000 milliseconds, or the parent drops out at roughly 1 second. You could always use an Infinite timeout here to wait until the child thread has completed processing.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
Pete O'Hanlon wrote:
by doing this you can wake the parent up if you finish processing earlier than 1000 milliseconds
That's the problem. SendMessage will almost certainly take less that 1000 ms, but if I send the next message straight away the server will get overloaded. The server can only handle one thing at a time, it accepts a message (and acknowledges it's been received). Then goes off and does whatever you told it to do, which can take a while, and then it becomes ready to receive the next message. If you send a second message while it's still handling the first it blows up.
Simon
-
MS suggests using goto to get out of deeply nested loops [^] :^) . So I suppose using it here should have no issues.
There's nothing left in my right brain and nothing right in my left brain.
The deep nesting is unnecessary as far as this particular piece of code is concerned. I'm sure it could be refactored. For instance, a simple while loop with a break condition must reduce a bit of the clutter and remove the
gotoalso.“Follow your bliss.” – Joseph Campbell
-
It looks OK in this case. The problem with
Thread.Sleepis that it is not guaranteed to sleep for the specified time. It may take few ticks more than what specified. But since you don't care about exact timings, it should be a valid usage. One alternative method is to useWaitHandleand pass a timeout value toWaitOnemethod.Best wishes, Navaneeth
N a v a n e e t h wrote:
use WaitHandle and pass a timeout value to WaitOne method.
Yeah, I suppose that's a way of avoiding Thread.Sleep, but it's essentially the same thing. I'd just be blocking the thread at the WaitOne until the timeout as I would never be signalling the wait handle because the server never sends any kind of message to let me know it's ready. I just have to pause for a bit and hope it's ready if I give it long enough.
Simon
-
Pete O'Hanlon wrote:
by doing this you can wake the parent up if you finish processing earlier than 1000 milliseconds
That's the problem. SendMessage will almost certainly take less that 1000 ms, but if I send the next message straight away the server will get overloaded. The server can only handle one thing at a time, it accepts a message (and acknowledges it's been received). Then goes off and does whatever you told it to do, which can take a while, and then it becomes ready to receive the next message. If you send a second message while it's still handling the first it blows up.
Simon
So, you've got a fragile condition in there then. You've got hacking around code in the client to cope with the fact that the server has a fairly fundamental limitation. What happens if the server takes longer than 1 second to process the message? It sounds like it blows up, so the Thread.Sleep is giving the client code a false sense of confidence here.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
-
So, you've got a fragile condition in there then. You've got hacking around code in the client to cope with the fact that the server has a fairly fundamental limitation. What happens if the server takes longer than 1 second to process the message? It sounds like it blows up, so the Thread.Sleep is giving the client code a false sense of confidence here.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
Pete O'Hanlon wrote:
So, you've got a fragile condition in there then.
Tell me about it.
Pete O'Hanlon wrote:
What happens if the server takes longer than 1 second to process the message? It sounds like it blows up
Yup. Boom. Fortunately the server is over the in the IT department so the shrapnel doesn't reach as far as my desk. (Incidentally, the 1 second figure was made up. In reality the delays depend on the commends sent, and are high estimates to allow for unexpected delays)
Pete O'Hanlon wrote:
the server has a fairly fundamental limitation.
Yup. A very big limitation, but unfortunately, there is absolutely no option for replacement, rewriting or changing anything about that server. Believe me, I've tried very very hard, it's out of my control. It's partly historical and partly political, and that combination makes it pretty much impossible to change.
Pete O'Hanlon wrote:
Thread.Sleep is giving the client code a false sense of confidence here.
Trust me, I have no confidence here false or otherwise, it's a horrible setup and I know it. Everything is done under the expectation that it's likely to fail. I do handle various failure cases. For example, if you do send the messages too quickly, the server hangs and stops responding to subsequent messages, so I handle timeouts during SendMessage, and abort the sequence. So I can handle a failure fairly well and notify the user that it didn't work. Fortunately, because it is only this one app that connects to the server, it's load is fairly predictable, so it's pretty rare that it exceeds expected times, and when it does all go bad there is an auto repair service that essentially restarts it and can bring it back online in around 5 minutes. So anyway. Valid use of Thread.Sleep then? :laugh:
Simon
-
Pete O'Hanlon wrote:
So, you've got a fragile condition in there then.
Tell me about it.
Pete O'Hanlon wrote:
What happens if the server takes longer than 1 second to process the message? It sounds like it blows up
Yup. Boom. Fortunately the server is over the in the IT department so the shrapnel doesn't reach as far as my desk. (Incidentally, the 1 second figure was made up. In reality the delays depend on the commends sent, and are high estimates to allow for unexpected delays)
Pete O'Hanlon wrote:
the server has a fairly fundamental limitation.
Yup. A very big limitation, but unfortunately, there is absolutely no option for replacement, rewriting or changing anything about that server. Believe me, I've tried very very hard, it's out of my control. It's partly historical and partly political, and that combination makes it pretty much impossible to change.
Pete O'Hanlon wrote:
Thread.Sleep is giving the client code a false sense of confidence here.
Trust me, I have no confidence here false or otherwise, it's a horrible setup and I know it. Everything is done under the expectation that it's likely to fail. I do handle various failure cases. For example, if you do send the messages too quickly, the server hangs and stops responding to subsequent messages, so I handle timeouts during SendMessage, and abort the sequence. So I can handle a failure fairly well and notify the user that it didn't work. Fortunately, because it is only this one app that connects to the server, it's load is fairly predictable, so it's pretty rare that it exceeds expected times, and when it does all go bad there is an auto repair service that essentially restarts it and can bring it back online in around 5 minutes. So anyway. Valid use of Thread.Sleep then? :laugh:
Simon
Ouch.
Simon Stevens wrote:
Valid use of Thread.Sleep then?
Yup.
"WPF has many lovers. It's a veritable porn star!" - Josh Smith
As Braveheart once said, "You can take our freedom but you'll never take our Hobnobs!" - Martin Hughes.
-
... I used my first goto today :shudders: to workaround this problem[^] I've been having.
private void BroadcastPacket(string Packet, FSClient client) { List ClientsToRemove = new List(); ASCIIEncoding encoder = new ASCIIEncoding(); byte\[\] PacketBytes = encoder.GetBytes(Packet); lock (ClientLock) { foreach (FSClient CurrentClient in clients) { if (client != CurrentClient) { try { if (CurrentClient.Socket.Connected) { TryAgain: try { CurrentClient.Socket.Send(PacketBytes, PacketBytes.Length, SocketFlags.None); bytesSent += PacketBytes.Length; } catch (SocketException ex) { if (ex.SocketErrorCode == SocketError.WouldBlock) { Console.WriteLine("SocketError.WouldBlock Occured"); Thread.Sleep(300); goto TryAgain; } } } else { ClientsToRemove.Add(CurrentClient); } } catch (Exception ex) { Console.WriteLine("Error @ BroadcastPacket(): " + ex.Message); ClientsToRemove.Add(CurrentClient); } } } } foreach (FSClient CurrentClient in ClientsToRemove) { CloseConnection(CurrentClient); } }I'm going straight to hell :((
Yes you are. GOTO hell, Go Directly to Hell, do not pass Go, do not collect $200. I'd certainly refactor - firstly as a number of smaller methods, which makes the structure far more readable and allows for refactoring if necessary, and secondly usng a loop structure in palce of the goto. But that's just me
___________________________________________ .\\axxx (That's an 'M')
