0

When setting up my socket for connecting, I wrap the method in a try-catch block..

If a socket exception gets raised, I create a new thread.. sleep for 15 seconds.. and then call the connect method again but this time from another thread. I'm doing this mainly for the sleep method (to avoid using a timer to reconnect) to not hang up the main thread.

Anyhow.. when trying to connect, I write the status to a text box using a method called Write() which just appends the text to the current text with a \n before it...

Because on a failed connect I create a separate thread to call the connect method (which DOES modify a control on the form), I am right to use invoke on the method call right?

Here is my code

private void Connect()
    {
        try
        {
            Write("Connecting...");
            _ClientSocket.Connect(new IPEndPoint(IPAddress.Loopback, 2500));
            Connected = true;
            Write("Connected.");
            _ClientSocket.BeginReceive(Buffer, 0, Buffer.Length, 0, new AsyncCallback(RecieveCallBack), null);
        }
        catch (SocketException ex)
        {
            Write("Connection Failed. Trying again in 15 Seconds...");
            Connected = false;
            new Thread(delegate()
            {
                Thread.Sleep(15000);
                Invoke((MethodInvoker)delegate
                {
                    this.Connect();
                });
            }).Start();
        }
        catch (Exception ex)
        {

        }
    }

I just want to be sure I am doing this in the proper way

Spencer Waz
  • 155
  • 8
  • 1
    testing should answer your question. Does it work without the invoke? Does it work with the invoke? If the answer is No and yes respectively then it would appear you have done the right thing. If the answer is yes and yes then you didn't need the invoke. – Chris Jul 25 '14 at 12:00
  • What is your UI framework? I am pretty sure this way of doing thing is wrong especially in the C# 5.0 world – Stilgar Jul 25 '14 at 12:02
  • I dont see where you access the WinForms. Also do not create threads manually. Use for example ThreadPool.QueueUserWorkItem – stepandohnal Jul 25 '14 at 12:02
  • This is a bad idea. Beyond forgetting to use BeginInvoke(), you can create additional bugs by flat-out assuming that the socket is connected after the Connect() call. So byte the bullet and *always* start a thread. Now everything is obvious, including the answer to the question. – Hans Passant Jul 25 '14 at 12:45
  • @HansPassant So your suggesting to skip connecting in the main thread at all and connecting in a separate thread always? – Spencer Waz Jul 25 '14 at 12:53
  • @stepandohnal in the question I said that the Write() method simply appends text to a textbox with a \n character – Spencer Waz Jul 25 '14 at 12:56

2 Answers2

1

In the scenario presented, it is using new Thread, Thread.Sleep and Invoke simply as a way of schedule some work to happen on the UI thread in 15 seconds. It'll work, but... pretty inefficient (threads are expensive). Frankly, a timer should be used instead - or perhaps Task.Delay on 4.5 (which actually just wraps a timer anyway).

Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
1

Instead of creating thread to connect you could initially assume, what your connect will not be successful. This will required to have polling timer to reconnect. Bonuses: you can control timer (while you can do nothing with anonymous thread), you can use it for other tasks required polling (resend data, which was not delivered at first try, queue connect after disconnect if you change socket settings, etc).

This polling timer can be a normal UI timer (Timer in case of winforms), then you don't need any invoke. If you go this way, then make sure don't have blocking operations in it (to example, sending data and waiting for an answer).

Otherwise, you can use this extension method for methods to always run them in UI thread

public static void InvokeIfRequired(this Control control, MethodInvoker action)
{
    if (control.InvokeRequired)
        control.Invoke(action);
    else
        action();
}

In your case you will want to call Write like this

someUIControl.InvokeIfRequired(() => Write(...));

Or simply make Write method like this

void Write(...)
{
    if(someUIControl.InvokeRequired)
        someUIControl.Invoke((delegate)() => Write(...));
    else
    {
        ... job here
    }
}
Community
  • 1
  • 1
Sinatr
  • 20,892
  • 15
  • 90
  • 319
  • On top of the Invoke answers, I appreciate the suggestion for the polling timer. I see how this could be more useful. – Spencer Waz Jul 25 '14 at 12:49