Is this thread.abort() normal and safe?

Question

I created a custom autocomplete control, when the user press a key it queries the database server (using Remoting) on another thread. When the user types very fast, the program must cancel the previously executing request/thread.

I previously implemented it as AsyncCallback first, but i find it cumbersome, too many house rules to follow (e.g. AsyncResult, AsyncState, EndInvoke) plus you have to detect the thread of the BeginInvoke'd object, so you can terminate the previously executing thread. Besides if I continued the AsyncCallback, there's no method on those AsyncCallbacks that can properly terminate previously executing thread.

EndInvoke cannot terminate the thread, it would still complete the operation of the to be terminated thread. I would still end up using Abort() on thread.

So i decided to just implement it with pure Thread approach, sans the AsyncCallback. Is this thread.abort() normal and safe to you?

public delegate DataSet LookupValuesDelegate(LookupTextEventArgs e);

internal delegate void PassDataSet(DataSet ds);

public class AutoCompleteBox : UserControl
{
   Thread _yarn = null;

   [System.ComponentModel.Category("Data")]
   public LookupValuesDelegate LookupValuesDelegate { set; get; }

   void DataSetCallback(DataSet ds)
   {
      if (this.InvokeRequired)
         this.Invoke(new PassDataSet(DataSetCallback), ds);
      else
      {
         // implements the appending of text on textbox here
      }
   }

   private void txt_TextChanged(object sender, EventArgs e)
   {
      if (_yarn != null) _yarn.Abort();

      _yarn = new Thread(
         new Mate
         {
            LookupValuesDelegate = this.LookupValuesDelegate,
            LookupTextEventArgs =
            new LookupTextEventArgs
            {
               RowOffset = offset,
               Filter = txt.Text
            },
            PassDataSet = this.DataSetCallback
         }.DoWork);

      _yarn.Start();
   }
}


internal class Mate
{
   internal LookupTextEventArgs LookupTextEventArgs = null;

   internal LookupValuesDelegate LookupValuesDelegate = null;

   internal PassDataSet PassDataSet = null;


   object o = new object();
   internal void DoWork()
   {
      lock (o)
      {
         // the actual code that queries the database
         var ds = LookupValuesDelegate(LookupTextEventArgs);
         PassDataSet(ds);
      }
   }
}

NOTES

The reason for cancelling the previous thread when the user type keys in succession, is not only to prevent the appending of text from happening, but also to cancel the previous network roundtrip, so the program won't be consuming too much memory resulting from successive network operation.

I'm worried if I avoid thread.Abort() altogether, the program could consume too much memory.

here's the code without the thread.Abort(), using a counter:

internal delegate void PassDataSet(DataSet ds, int keyIndex);

public class AutoCompleteBox : UserControl
{
   [System.ComponentModel.Category("Data")]
   public LookupValuesDelegate LookupValuesDelegate { set; get; }

   static int _currentKeyIndex = 0;

   void DataSetCallback(DataSet ds, int keyIndex)
   {
      if (this.InvokeRequired)
         this.Invoke(new PassDataSet(DataSetCallback), ds, keyIndex);
      else
      {
         // ignore the returned DataSet
         if (keyIndex < _currentKeyIndex) return; 

         // implements the appending of text on textbox here...
      }
   }

   private void txt_TextChanged(object sender, EventArgs e)
   {
      Interlocked.Increment(ref _currentKeyIndex);

      var yarn = new Thread(
         new Mate
         {
            KeyIndex = _currentKeyIndex,
            LookupValuesDelegate = this.LookupValuesDelegate,
            LookupTextEventArgs =
            new LookupTextEventArgs
            {
               RowOffset = offset,
               Filter = txt.Text
            },
            PassDataSet = this.DataSetCallback
         }.DoWork);

      yarn.Start();
   }
}


internal class Mate
{
   internal int KeyIndex;
   internal LookupTextEventArgs LookupTextEventArgs = null;
   internal LookupValuesDelegate LookupValuesDelegate = null;
   internal PassDataSet PassDataSet = null;

   object o = new object();
   internal void DoWork()
   {
      lock (o)
      {
         // the actual code that queries the database
         var ds = LookupValuesDelegate(LookupTextEventArgs);
         PassDataSet(ds, KeyIndex);
      }
   }
}

Answer 1

You might want to have a look at An Introduction to Programming with C# Threads - Andrew D. Birrell. He outlines some of the best practices surrounding threading in C#.

On page 4 he says:

When you look at the “System.Threading” namespace, you will (or should) feel daunted by the range of choices facing you: “Monitor” or “Mutex”; “Wait” or “AutoResetEvent”; “Interrupt” or “Abort”? Fortunately, there’s a simple answer: use the “lock” statement, the “Monitor” class, and the “Interrupt” method. Those are the features that I’ll use for most of the rest of the paper. For now, you should ignore the rest of “System.Threading”, though I’ll outline it for you section 9.

Answer 2

No, it is not safe. Thread.Abort() is sketchy enough at the best of times, but in this case your control has no (heh) control over what's being done in the delegate callback. You don't know what state the rest of the app will be left in, and may well find yourself in a world of hurt when the time comes to call the delegate again.

Set up a timer. Wait a bit after the text change before calling the delegate. Then wait for it to return before calling it again. If it's that slow, or the user is typing that fast, then they probably don't expect autocomplete anyway.

Regarding your updated (Abort()-free) code:

You're now launching a new thread for (potentially) every keypress. This is not only going to kill performance, it's unnecessary - if the user isn't pausing, they probably aren't looking for the control to complete what they're typing.

I touched on this earlier, but P Daddy said it better:

You'd be better off just implementing a one-shot timer, with maybe a half-second timeout, and resetting it on each keystroke.

Think about it: a fast typist might create a score of threads before the first autocomplete callback has had a chance to finish, even with a fast connection to a fast database. But if you delay making the request until a short period of time after the last keystroke has elapsed, then you have a better chance of hitting that sweet spot where the user has typed all they want to (or all they know!) and is just starting to wait for autocomplete to kick in. Play with the delay - a half-second might be appropriate for impatient touch-typists, but if your users are a bit more relaxed... or your database is a bit more slow... then you may get better results with a 2-3 second delay, or even longer. The most important part of this technique though, is that you reset the timer on every keystroke.

And unless you expect database requests to actually hang, don't bother trying to allow multiple concurrent requests. If a request is currently in-progress, wait for it to complete before making another one.

Answer 3

There are many warnings all over the net about using Thread.Abort. I would recommend avoiding it unless it's really needed, which in this case, I don't think it is. You'd be better off just implementing a one-shot timer, with maybe a half-second timeout, and resetting it on each keystroke. This way your expensive operation would only occur after a half-second or more (or whatever length you choose) of user inactivity.

Answer 4

No, I would avoid ever calling Thread.Abort on your own code. You want your own background thread to complete normally and unwind its stack naturally. The only time I might consider calling Thread.Abort is in a scenario where my code is hosting foreign code on another thread (such as a plugin scenario) and I really want to abort the foreign code.

Instead, in this case, you might consider simply versioning each background request. In the callback, ignore responses that are "out-of-date" since server responses may return in the wrong order. I wouldn't worry too much about aborting a request that's already been sent to the database. If you find your database isn't responsive or is being overwhelmed by too many requests, then consider also using a timer as others have suggested.