TcpClient disposed prematurely

Question

I have a problem I cannot seem to figure out, please help. I have created a class to handle an interface to some HW using TcpClient. I want this class to send one last command to the HW before it is destroyed.

To solve this I have implemented IDisposable.Dispose to take care of the sending of the last command and then close the connection. I have also in the destructor made a call to Dispose. This is the Microsoft recommendation as I read it in this article. My code is as follows:

class MyHWInterface : IDisposable
{
    ~MyHWInterface()
    {
        Dispose();
    }

    private bool disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (!disposed)
        {
            CloseConnection();
            disposed = true;
        }
    }

    private System.Net.Sockets.TcpClient Port = new System.Net.Sockets.TcpClient();

    public bool OpenConnection()
    {
        ...
    }

    private bool SendCommand(string command)
    {
        var strm = Port.GetStream(); // This throws the exception Cannot access disposed object!
        var w = new System.IO.StreamWriter(strm, System.Text.Encoding.ASCII);
        var r = new System.IO.StreamReader(strm, System.Text.Encoding.ASCII);
        w.WriteLine(command);
        w.Flush();
        string l = r.ReadLine();
        return l == "OK";
    }

    internal void CloseConnection()
    {
        try
        {
            SendCommand("power down now");
        }
        catch
        {
        }
        finally
        {
            Port.Close();
        }
    }
}

My problem is: When my program ends, and my object of MyHWInterface therefore falls out of scope and then gets garbage collected. The destructor is called which tries to send the last command, which fails because somehow my TcpClient is already disposed.

Please tell me why an object which is clearly not yet out of scope is being disposed. And please help with a method that makes sure my last command always will be send without an explicit call to Dispose.

You should not have a finalizer. Also, you're implementing the `Dispose()` pattern incorrectly. — SLaks, Jan 20 '15 at 16:49

score 4 · Accepted Answer · answered Jan 20 '15 at 16:46

4

Please tell me why an object which is clearly not yet out of scope is being disposed.

Objects don't have a concept of "scope" as such. At the end of your program, both the TcpClient and the instance of your class are eligible for finalization - and there's no guarantee which will be finalized first. It sounds like the TcpClient is being finalized (and the connection closed) first, hence the issue.

The best fix is not to rely on finalization for this in the first place - remove your own finalizer (realizing that the connection will just be closed at some point if the client doesn't call Dispose) and make sure that you do dispose of everything in an orderly fashion when your program terminates, assuming it's terminated cleanly (i.e. through some path you control).

answered Jan 20 '15 at 16:46

Jon Skeet

1,421,763
867
9,128
9,194

It's always bothered me that the 'standard Dispose pattern' includes a finalizer because of possible inheritance, since finalizers are almost always a bad idea. My solution is to make pretty much all Disposable classes sealed, so there's no need to worry about inheritance and hence Dispose can be much simpler. – Dan Bryant Jan 20 '15 at 16:55
@DanBryant: Yes, likewise. I favour sealed classes in general, regardless of implementing `IDisposable`, mind you :) – Jon Skeet Jan 20 '15 at 17:01
@Kaare: I agree with Jon's diagnosis. However, I will differ slightly on the specific recommendation: while I agree that a finalizer is probably not needed, the reason for that is that the class doesn't keep any unmanaged resources (which IMHO is the only reason to implement a finalizer...and frankly, it's better to use `SafeHandle` to manage unmanaged resources anyway). The bigger issue here is that you are using the `IDisposable` pattern to handle an unrelated issue, effecting one final operation. Don't do that...just implement that explicitly and leave `IDisposable` out of it. – Peter Duniho Jan 20 '15 at 17:54
@PeterDuniho: It's one final operation in this particular example, but I see no obvious reason why a different program shouldn't use an instance of `MyHWInterface`, dispose of it, then use a new one etc - I don't see anything really tying this to program lifetime, but each instance should still be disposed. It seems entirely reasonable to implement `IDisposable` here, not only to suggest to clients that they *should* clean up after themselves, but also to make it easier for them to do so safely (with a `using` statement) – Jon Skeet Jan 20 '15 at 18:02
My point is that the `IDisposable` implementation here carries extra baggage: a complete transaction on the `TcpClient` object. I'm all for explicitly cleaning up the `TcpClient` itself, but I'd rather see the `Dispose()` method nice and simple, with no chance of throwing exceptions, and without the delay caused by the extra network I/O (never mind the extra objects allocated during the dispose operation to handle that I/O). I.e. all it ought to do is call `Port.Close();` – Peter Duniho Jan 20 '15 at 18:06
1

@PeterDuniho: I see. Although I'd point out that closing an unflushed `FileStream` could potentially throw in the same way while flushing. I guess it partly depends on what the OP wants to do if the instance is disposed as part of an exception propagating up - at that point, the "power down now" command may not be suitable. Hmm. Definitely thought required on the part of the OP :) – Jon Skeet Jan 20 '15 at 18:08
Basically I agree that my program in principle should explicitly clean up after it is done using the MyHWInterface. However my reasoning for wanting to send the one last command " power down now" is to clean up the resources in my HW. Just as a FileStream closes the file when the program ends even if I forgot to do so explicitly. I was hoping I could do cleaning in the destructor, and I still do not understand that the TcpClient is being finalized while it is still referenced by my object. – Kaare Jan 21 '15 at 09:13
@Kaare: It's being finalized because your object is *also* eligible for garbage collection. When a bunch of objects are eligible for garbage collection, there's no rule about which gets finalized first. (Suppose you tried to do that. Imagine there were two objects with references to each other - which of those should be finalized first?) – Jon Skeet Jan 21 '15 at 09:19
@Jon: I guess my problem has been, that I did not expect the TcpClient being eligble for garbage collection since I refer to it in my destructor. I found this MS document [link](https://msdn.microsoft.com/en-us/library/aa691138%28v=vs.71%29.aspx) which does say destructors are not considered as valid reason for waiting to garbage collect fields (see first part of point 2). At least now I understand why I cannot access my field in my destructor. Unfortunately I do not have a good solution to my problem. – Kaare Jan 22 '15 at 12:57
Again, thank you all for taking the time to get me understand the issues here. – Kaare Jan 22 '15 at 12:58

TcpClient disposed prematurely

1 Answers1