How to abort a thread properly?

Question

I'm writing a scripting interface for a server emulator for NPC chats. The player can initiate a NPC chat by clicking on the NPC. The NPC can send text dialogs. Those text dialogs also contain a End Chat button to end the chat before the script finishes execution, or the player can continue with the text dialogs normally until they end.

When the player interrupts the chat, a special packet is sent.

I've created a class called WaitableResult which takes advantage of ManualResetEvent to block the current thread until given result and then returns the result:

public sealed class WaitableResult<T> where T : struct
{
    public T Value { get; private set; }

    private ManualResetEvent mEvent;

    public WaitableResult()
    {
        mEvent = new ManualResetEvent(false);
    }

    public void Wait()
    {
        mEvent.WaitOne();
    }

    public void Set(T value)
    {
        mEvent.Set();

        this.Value = value;
    }
}

This is my script classes:

internal sealed class NpcScript : ScriptBase
{
    public WaitableResult<bool> BoolResult { get; private set; }

    private Npc mNpc;
    private Player mPlayer;

    public NpcScript(Npc npc, Player player)
        : base(string.Format(@"..\..\scripts\npcs\{0}.lua", npc.Script), true)
    {
        mNpc = npc;
        mPlayer = player;
        mPlayer.NpcConversation = this;

        this.Expose("answer_no", false);
        this.Expose("answer_yes", true);
        this.Expose("answer_decline", false);
        this.Expose("answer_accept", true);

        this.Expose("say", new Func<string, bool>(this.Say));
        this.Expose("askYesNo", new Func<string, bool>(this.AskYesNo));
    }

    public override void Dispose()
    {
        base.Dispose();

        mPlayer.NpcConversation = null;
    }

    private bool Say(string text)
    {
        this.BoolResult = new WaitableResult<bool>();

        using (OutPacket outPacket = mNpc.GetDialogPacket(ENpcDialogType.Standard, text, 0, 0))
        {
            mPlayer.Client.SendPacket(outPacket);
        }

        this.BoolResult.Wait();

        return this.BoolResult.Value;
    }

    private bool AskYesNo(string text)
    {
        this.BoolResult = new WaitableResult<bool>();

        using (OutPacket outPacket = mNpc.GetDialogPacket(ENpcDialogType.YesNo, text))
        {
            mPlayer.Client.SendPacket(outPacket);
        }

        this.BoolResult.Wait();

        return this.BoolResult.Value;
    }
}

 public abstract class ScriptBase
{
    private string mPath;
    private Thread mThread;
    private MoonSharp.Interpreter.Script mScript;


    public ScriptBase(string path, bool useThread = false, CoreModules modules = CoreModules.None)
    {
        mPath = path;
        if (useThread) mThread = new Thread(new ThreadStart(() => mScript.DoFile(mPath)));
        mScript = new MoonSharp.Interpreter.Script(modules);
    }

    public void Execute()
    {
        if (mThread != null)
        {
            mThread.Start();
        }
        else
        {
            mScript.DoFile(mPath);
        }
    }

    public virtual void Dispose()
    {
        if (mThread != null)
        {
            mThread.Abort();
            mThread = null;
        }
    }

    protected void Expose(string key, object value)
    {
        mScript.Globals[key] = value;
    }
}

And here's an example of a script:

say('test')
say('some more stuff')
say('good bye')

When a player initiates a chat with a NPC and finishes it without interrupting it (aka closing it using the End Chat button), the thread should be aborted by itself (as it finished all it's instructions).

However, when a player aborts the chat before it finishes execution, I'm calling the Dispose button to manually abort the thread - but I'm not sure it's the right way to do it.

My memory usage also increases by 1 MB everytime a player starts a chat, so that's also kind of weird.

Answer 1

As Quantic has already addressed, aborting a thread by means of Thread.Abort invites unexpected - even dangerous results. Quantic also addressed the preferred pattern to deal with a thread, usually the thread loops and checks for a termination flag, which is not possible if you are executing an arbitrary Lua script.

One option you have is to create a minimalistic debugger and attach it to your script. Once this debugger is attached, the IDebugger.GetAction() method will be called for every instruction in the running script. If during the GetAction()call you raise an Exception, the script will be terminated, cleanly, by the debugger.

Here is an example of a sentinel, minimalistic debugger that stops a long-running script using such a technique. As you can see, most of the implementation is empty. In your case, instead of using a running instruction counter, you may simply have a boolean flag that indicates that the script must be terminated.

public DebuggerAction GetAction(int ip, SourceRef sourceref)
{
    if( _abortScript )          
       throw new MyException();  // abort cleanly

    // Proceed running the next statement
    return new DebuggerAction() { 
      Action = DebuggerAction.ActionType.StepIn,
    };
}

Regarding the memory increase you are experiencing, that is one of the inconveniences of manually spawning threads, each thread initializes its own stack space and on a 32-bit process, the default is precisely 1MB. There is a Thread constructor overload that allows you to customize the stack size; Depending on the complexity of the scripts, this value could probably be set to a smaller value without risking a stack overflow condition. Another option to minimize memory usage would be to use a Task.Run instead of spawning your own thread (this makes use of the Thread pool), but then refraining from using Thread.Abort would be mandatory, you don't want to go killing pooled threads!