Is this thread.abort() normal and safe?

Solution 1:

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.

Solution 2:

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.

Solution 3:

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.

Solution 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.