35

Below is some sample code written by a colleague. This seems obviously wrong to me but I wanted to check. Should an object call its own Dispose() method from within one of its own methods? It seems to me that only the owner/creator of the object should call Dispose() when it's done with the object and not the object itself.

It's an .asmx web method that calls Dispose() on itself when it's done. (The fact that it's a web method is probably incidental to the question in general.) In our code base we sometimes instantiate web service classes within methods of other web services and then call methods on them. If my code does that to call this method, the object is toast when the method returns and I can't really use the object any more.

[WebMethod]
public string MyWebMethod()
{
    try
    {
        return doSomething();
    }
    catch(Exception exception)
    {
        return string.Empty;
    }
    finally
    {
        Dispose(true);
    }
}

UPDATE: Found a few links that are related:

Do I need to dispose a web service reference in ASP.NET?

Dispose a Web Service Proxy class?

Community
  • 1
  • 1
Tom Winter
  • 1,813
  • 3
  • 18
  • 23
  • 2
    Get the colleague to explain the reasoning behind this and we might help you with arguments on why not to do this :D – David Mårtensson Feb 18 '11 at 18:52
  • @David: There are certain cases where an object is serializable but not clonable; serializing an instance invalidates it, and any serialized representation can only be deserialized once. TCP sockets are like that. – supercat Feb 18 '11 at 20:20
  • @David: I don't believe my colleague really understands what Dispose is for. We have a large object library, and all the classes keep an instance of a utility class that implements IDispose, just so it can set a Dictionary<> instance to null! It's frustrating to have to use using() all the time when it's not really needed. And don't get me started on all the return codes instead of exceptions. Of course I didn't know everything when I started with .Net. ;-) – Tom Winter Feb 18 '11 at 21:34
  • 1
    Explain to your colleague that the purpose of IDisposable isn't to make an object go away, but rather to perform cleanup on entities *outside* the object. An object should implement IDisposable if it's going to be the last holder of the information and impetus necessary to perform such cleanup. Generally speaking, if an object's Dispose method isn't going to clean up entities outside itself, it may as well not exist. – supercat Feb 18 '11 at 21:49

8 Answers8

36

For sure it's not a good prartice. The caller should decide when he is finished using the IDisposable object, not an object itself.

EvgK
  • 1,907
  • 12
  • 10
  • 1
    I agree, the process creating the object has to be able to rely on it being available and not suddenly get an error message like Null pointer exception that will be very difficult to debug. – David Mårtensson Feb 18 '11 at 18:51
  • 3
    @David [ObjectDisposedException](http://msdn.microsoft.com/en-us/library/system.objectdisposedexception.aspx) in this case. – Adam Lear Feb 18 '11 at 18:55
  • 2
    The proper semantics for calling Dispose is "Would the last one to leave the room please turn out the lights". Objects should not dispose themselves if other objects still need them, but if an object finds out that it's not needed anymore, and nobody else is going to dispose it in timely fashion, the object should dispose itself. – supercat Feb 18 '11 at 20:10
  • @supercat: I disagree with your last sentence. C# has garbage collection in order to take care of object disposal when it's most convenient for the program; and it's pretty good at it. The ONLY time you should override this is if the object deals with unmanaged resources; which gets back to the code responsible for instantiating the object should be responsible for releasing it, not the object itself. – NotMe Feb 18 '11 at 20:22
  • 1
    @Chris Lively: If an object manipulates the state of some entity outside itself in a fashion that will require cleanup, such cleanup should generally be performed as soon as the object knows that it won't interfere with what it wants to do. Code should never rely upon finalizers for cleanup except when there's a good reason (e.g. there will be a small number of widely-shared objects, and it would be difficult to know when every last user of an object has abandoned it). Many things can prevent finalizers from running in timely fashion, so deterministic disposal is almost always better. – supercat Feb 18 '11 at 20:45
8

There are very few valid reasons to perform a "Self Disposing" action. Threading is the one I use more than not.

I have several applications that "fire and forget" threads. Using this methodology allow the object to self dispose.

This helps keep the environment clean without a threading manager process.

Matthew R.
  • 4,332
  • 1
  • 24
  • 39
Tuk
  • 81
  • 1
  • 1
4

if I ever see that in one of my projects, I would ask why and I'm 99.9999% sure that i would remove it anyway

for me this is a kind of red flag / code smells

Fredou
  • 19,848
  • 10
  • 58
  • 113
1

There are no technical restrictions on what a Dispose method is allowed to do. The only thing special about it is that Dispose gets called in certain constructs (foreach, using). Because of that, Dispose might reasonably be used to flag an object as no-longer-useable, especially if the call is idempotent.

I would not use it for this purpose however, because of the accepted semantics of Dispose. If I wanted to mark an object as no-longer-useable from within the class itself, then I would create a MarkUnuseable() method that could be called by Dispose or any other place.

By restricting the calls to Dispose to the commonly accepted patterns, you buy the ability to make changes to the Dispose methods in all of your classes with confidence that you will not unexpectedly break any code that deviates from the common pattern.

Jeffrey L Whitledge
  • 58,241
  • 9
  • 71
  • 99
0

Just remove it, but take care to dispose it in all object that call it.

Guilherme de Jesus Santos
  • 4,955
  • 5
  • 43
  • 68
  • The only use its getting is when a web browser calls the web method. IIS/ASP is somehow creating the object, so it should be the one to Dispose of it I assume. – Tom Winter Feb 18 '11 at 21:19
0

Technically yes, if that "method" is the finaliser and you are implementing the Finalise and IDisposable pattern as specified by Microsoft.

Slugart
  • 4,535
  • 24
  • 32
0

While a .Net object would not normally call Dispose on itself, there are times when code running within an object may be the last thing that expects to be using it. As a simple example, if a Dispose method can handle the cleanup of a partially-constructed object, it may be useful to have a constructor coded something like the following:

Sub New()
  Dim OK As Boolean = False
  Try
    ... do Stuff
    OK = True
  Finally
    If Not OK Then Me.Dispose
  End Try
End Sub

If the constructor is going to throw an exception without returning, then the partially-constructed object, which is slated for abandonment, will be the only thing that will ever have the information and impetus to do the necessary cleanup. If it doesn't take care of ensuring a timely Dispose, nothing else will.

With regard to your particular piece of code, the pattern is somewhat unusual, but it looks somewhat like the way a socket can get passed from one thread to another. There's a call which returns an array of bytes and invalidates a Socket; that array of bytes can be used in another thread to create a new Socket instance which takes over the communications stream established by the other Socket. Note that the data regarding the open socket is effectively an unmanaged resource, but it can't very well be wrapped in an object with a finalizer because it's often going to be handed off to something the garbage-collector can't see.

supercat
  • 77,689
  • 9
  • 166
  • 211
  • I could see doing that in a constructor, but that's about it. – Tom Winter Feb 18 '11 at 21:35
  • A constructor or factory method would be the two most common situations I would think. As I noted elsewhere, another scenario would be code to hand off a live instance of something like a TCP socket. If the handoff is successful, the recipient will dispose the socket. If unsuccessful, and the calling code will not be expecting to maybe still have a live socket, the handoff method might have to take care of the disposal itself. – supercat Feb 20 '11 at 17:47
0

No! It is unexpected behavior and breaks the best practices guidelines. Never do anything that is unexpeceted. Your object should only do what is needed to maintain it's state while protecting the integrity of the object for the caller. The caller will decide when it's done (or the GC if nothing else).

Dustin Davis
  • 14,482
  • 13
  • 63
  • 119
  • @DustinDavis: How should one handle an something like a TCP socket which is effectively serializable, but not clonable, other than by having the socket dispose of itself in the serializing routine? If the object will be invalid after serialization, is there any reason it shouldn't dispose itself? – supercat Feb 18 '11 at 20:30
  • @supercat if your instance is going to be invalid after you deserialize it, then you should handle that on the deserialization process (either internally on the class or by a factory). If you serialize socket1 isntance and it disposed itself then socket1 is no longer usable by the code which would be confusing to a consumer and can cause unexpected behavior. – Dustin Davis Feb 18 '11 at 20:42
  • @DustinDavis: Serializing a socket instance is similar to 'parking' a call on an office phone system. When a call is parked, the display will say "Parked on 102" (or some other number); I can then announce on the PA "Bill--call on 102", then Bill can go to a phone, punch in "102", and take over the call. The first person to punch in the token "102" after I park the call will get the conversation, but nobody else can. If I wanted to I could punch "102" myself, but then nobody else could. Parking the call generally means I myself no longer have it *which is why I parked in the first place*. – supercat Feb 18 '11 at 20:51
  • @DustinDavis: BTW, the routine that serializes a socket is called DuplicateAndClose; someone who calls such a routine shouldn't be terribly surprised that the socket is no longer usable. I would expect that routines that perform similar actions in other scenarios should be similarly named. With regard to the original question, I'm guessing that the DoSomething() might be semantically similar to "DuplicateAndClose", and that the code above would be a wrapper to ensure that the object gets cleaned up even if the serialization fails. – supercat Feb 18 '11 at 20:55
  • @supercat if there is a method called DuplicateAndClose() then yes, there should be no suprise and in that case, it should be acceptable to dispose of what needs to be disposed of. I'm looking from general usage of a consumer doing a serialization. – Dustin Davis Feb 18 '11 at 20:58
  • @supercat: My DoSomething() isn't really anything special. It could just be returning a string from database. Nothing special is going on at all. – Tom Winter Feb 18 '11 at 21:16
  • @Tom Winter: If the code isn't doing anything special which would tightly attach the "return string" and "dispose object" functionality, then I don't see any reason for code like that shown in the original question. The pattern does resemble that for passing sockets from one process to another, and would be appropriate for that case, but it's not a good "general-purpose" pattern. – supercat Feb 18 '11 at 21:44