0
\$\begingroup\$

I have two threads, where one listens on TCP and the other renders in a loop:

  private void checkBox1_CheckedChanged(object sender, EventArgs e)
        {
            try
            {
                if (ReceiveCheck.Checked)
                {
                    tcplisten.Start();
                    ListenThread = new Thread(new ThreadStart(Listen));
                    ListenThread.Start();
                    RenderThread = new Thread(new ThreadStart(Render));
                    RenderThread.Start();

                }
                else
                {
                    tcplisten.Stop();
                    RenderThread.Abort();
                    ListenThread.Abort();
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message, "Checkbox");
            }

        }

Is this a good way of handling the threads, to just start them and later kill them when I want to?

\$\endgroup\$
  • \$\begingroup\$ Can you clarify this "To just start then, and kill them when i want to?". Are you asking how to do this or is this a good way to do this? \$\endgroup\$ – Aseem Bansal Aug 24 '13 at 11:44
  • 1
    \$\begingroup\$ Is there any reason you're not using the TPL? It's a very nice abstraction and it'll really help you in such cases. \$\endgroup\$ – Benjamin Gruenbaum Aug 24 '13 at 12:24
  • \$\begingroup\$ @Aseem Bansal I am asking if it´s good. \$\endgroup\$ – Zerowalker Aug 24 '13 at 13:01
  • \$\begingroup\$ @Benjamin Gruenbaum Never heard of, can you put an answer with what you mean, to display it? \$\endgroup\$ – Zerowalker Aug 24 '13 at 13:02
5
\$\begingroup\$

No, this is absolutely not a good way. Thread.Abort() should never be used, because it is very hard to write correct code when an exception can happen at almost any point in your code.

Instead, you should implement cooperative cancellation either by using a volatile bool flag, or, even better, CancellationToken.

With that your code could look like this:

Thread ListenThread;
Thread RenderThread;
CancellationTokenSource CTS;

private void checkBox1_CheckedChanged(object sender, EventArgs e)
{
    try
    {
        if (ReceiveCheck.Checked)
        {
            tcplisten.Start();
            CTS = new CancellationTokenSource();
            ListenThread = new Thread(() => Listen(CTS.Token)));
            ListenThread.Start();
            RenderThread = new Thread(() => Render(CTS.Token)));
            RenderThread.Start();
        }
        else
        {
            tcplisten.Stop();
            CTS.Cancel();
        }
    }
    catch (Exception ex)
    {
        MessageBox.Show(ex.Message, "Checkbox");
    }
}

Your Listen() and Render() methods would then periodically check IsCancellationRequested of the passed in token and return if it's true.

\$\endgroup\$
  • \$\begingroup\$ Good to know. Though, never used any of those CancellationToken or volatile bool. Can you show an example? \$\endgroup\$ – Zerowalker Aug 24 '13 at 13:02
  • \$\begingroup\$ @Zerowalker It's not complicated, see update. \$\endgroup\$ – svick Aug 24 '13 at 14:36
  • \$\begingroup\$ I tried that, but it doesn´t work. Am i supposed to implement the IsCancellationRequested myself? in While loop or something? \$\endgroup\$ – Zerowalker Aug 24 '13 at 15:11
  • \$\begingroup\$ @Zerowalker Like I said, you need to check IsCancellationRequested yourself. So, if you have a loop in your Render() method, then you should check it at the start of each iteration, or something like that. \$\endgroup\$ – svick Aug 24 '13 at 15:18
  • \$\begingroup\$ if i am supposed to do something like this: if (CTS.IsCancellationRequested) { break; } - Why can´t i just use my CheckBox value instead? \$\endgroup\$ – Zerowalker Aug 24 '13 at 15:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.