5
\$\begingroup\$

I have a question about using an IDisposable object in the ThreadStart function. I have a class that derives from ServiceBase:

public class ServiceTest: System.ServiceProcess.ServiceBase
{
    private System.ComponentModel.Container components = null;
    private System.Threading.Thread         m_cProcessThread;

    public ServiceTest()
    {
        this.ServiceName = "ServiceTest";
    }

    static void Main()
    {
        System.ServiceProcess.ServiceBase[] ServicesToRun  = new System.ServiceProcess.ServiceBase[] { new ServiceTest() };

        System.ServiceProcess.ServiceBase.Run(ServicesToRun);
    }
    protected override void Dispose( bool disposing )
    {
        if ( disposing )
        {
            if (components != null) 
            {
                components.Dispose();
            }
        }
        base.Dispose( disposing );
    }

    protected override void OnStart(string[] args)
    {
        ProcessThread   cThread;

        cThread = new ProcessThread();
        m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
        m_cProcessThread.Start();
    }

    protected override void OnStop()
    {
        m_cProcessThread.Abort();
    }
}

and in the OnStart function I have the ProcessThread object which is an IDisposable object. I was wondering if it is safe to re-write the OnStart function to do this:

    protected override void OnStart(string[] args)
    {
        using (Process cThread = new ProcessThread())
        {
            m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
        }

        m_cProcessThread.Start();
    }

Or will there be any consequences in doing this? I have a bit of an OCD when I see IDisposable objects in C# code that makes me believe that they HAVE to be disposed as soon as you are done lol. Also I don't know if my question title really fits with what I'm asking so if you could come up with a better title then feel free to edit.

Code is targeted at .net 2.0, running on a Windows 2000 server.

\$\endgroup\$
  • 1
    \$\begingroup\$ This may or may not be the best site to ask a question like this. Are you looking for a general code review, or just a specific answer to "Could I dispose IDisposable objects that are part of a Thread?" \$\endgroup\$ – Phrancis Nov 6 '14 at 18:10
  • \$\begingroup\$ It was the latter. I didn't want to post on StackOverflow because it is not a "my code don't work what is going on" question, so I thought this was the best place. \$\endgroup\$ – John Odom Nov 6 '14 at 19:17
  • \$\begingroup\$ Looks like you got good answers already so I guess it is fine to leave this question open. \$\endgroup\$ – Phrancis Nov 6 '14 at 19:54
7
\$\begingroup\$

Strictly speaking, it depends on what InitProcess does. It may very well work OK as long as it never touches resources in the ProcessThread class which get disposed.

However, even if it works this time, it is a bad idea. Passing a method group from a class which gets disposed before the thread even starts is asking for trouble.

A better option would be to create a method inside your ServiceTest class that the thread executes. Inside that thread, you can instantiate a ProcessThread object, call InitProcess, and ensure that Dispose is only called once InitProcess has completed:

protected override void OnStart(string[] args)
{
    m_cProcessThread = new Thread(InitProcess);
    m_cProcessThread.Start();
}

private void InitProcess()
{
    using(var processThread = new ProcessThread())
    {
        processThread.InitProcess();
    }
}

You may also note that I removed the explicit construction of a ThreadStart delegate. In many cases, you can pass a method group directly where delegates are called for. I find it a little cleaner to do so.

\$\endgroup\$
7
\$\begingroup\$
using (Process cThread = new ProcessThread())
{
    m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
}

This would dispose cThread right away, which IMO would make the next line...

m_cProcessThread.Start();

...possibly throw some ObjectDisposedException, depending on what's going on in InitProcess - see @DanLyons' answer.


Your code file should have the following using statements at the top:

using System.ServiceProcess;
using System.ComponentModel;
using System.Threading;

This would allow you to get rid of the full qualifications, so this:

System.ServiceProcess.ServiceBase[] ServicesToRun  = new System.ServiceProcess.ServiceBase[] { new ServiceTest() };

Can be shortened to that:

ServiceBase[] ServicesToRun = new ServiceBase[] { new ServiceTest() };

Or even:

var ServicesToRun = new ServiceBase[] { new ServiceTest() };

The naming style is confusing. Why all these prefixes?

private System.Threading.Thread         m_cProcessThread;

Should be simply:

private Thread processThread;

No need for all these whitespaces, no need for this m_ prefix, and no need for that cryptic c either.

Why isn't components following the same convention anyway? If you're prefixing all instance-level fields with an m_, then I would expect components to be called m_components - be consistent!

That said, as much as I don't like random and Hungarian prefixes, I like starting my private fields with an underscore, like _components and _processThread, so as to avoid having to qualify them with this later on when I use them in a method with a components or a processThread parameter... but that's just personal preference; all that matters, is consistency.

Now, you have a Thread object called m_cProcessThread, and then you have a ProcessThread object called cThread - that is utterly confusing! I'd call the Thread object thread, and the ProcessThread object, processThread.

The whitespace here is annoyingly inconsistent:

protected override void Dispose( bool disposing )
{
    if ( disposing )
    {
        if (components != null) 
        {
            components.Dispose();
        }
    }
    base.Dispose( disposing );
}

There is no reason to fight your IDE to get these extra whitespaces after ( and before ). This is much less disturbing to read:

protected override void Dispose(bool disposing)
{
    if (disposing)
    {
        if (components != null) 
        {
            components.Dispose();
        }
    }
    base.Dispose(disposing);
}

Your OCD about disposing disposables isn't necessarily a bad thing. The rule of thumb however, is that whichever scope is creating a disposable object, should be responsible for disposing it. This means your OnStart method should either be disposing the thread it's creating (with the issues mentioned above), or not be creating it at all.

\$\endgroup\$
  • \$\begingroup\$ Yeah, sorry for the inconsistency, but the m_c was a coding standard that I been following for quite some time. The m_ means it is a member variable, and c means that it is a class. \$\endgroup\$ – John Odom Nov 6 '14 at 19:08
  • \$\begingroup\$ Sorry for double-comment but this code is running on .NET 2.0 in a Windows 2000 server. If that helps any. \$\endgroup\$ – John Odom Nov 6 '14 at 19:16

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.