Always dispose the disposables!



  • I was trying to track down a serious bug, and I found this gem in a C# library maintained by a colleague. Class and variable names have been changed, of course.

            public Foo DoBar()
            {
                // create bletch instance
                using (Bletch bletch = new Bletch())
                {
                    Kaka kaka = bletch.getKaka();
                    if (kaka == null)
                    {
                        kaka.Dispose();
    

    I had a critical bug to resolve, not to mention that the colleague and my boss are both a little prickly about me touching this area of code, so I wrote an entry in our bug tracking system about a guaranteed NPE and went on.

    The colleague, instead of being slightly chagrined and removing the cruft, has lowered the priority from "Normal" to "Low".

    Note that this code is not frozen and gets edited on a regular basis. I am guessing, 'cause no one has said, that the rationalizations are "it would never happen" or "it has never happened" and almost certainly "I have real work to do, quit being a pain in the ass."

    This thing, it makes me sad. :(


  • Considered Harmful

    Just fix it. If anybody confronts you about it, they are an idiot.



  • Yes, just fix it, but also include the following comment:



    // Please see [url=http://forums.thedailywtf.com/forums/t/29134.aspx]http://forums.thedailywtf.com/forums/t/29134.aspx[/url] for why this code has been fixed.



    For the record... anyone who didn't want this to be fixed should immediately swap job roles with rstinejr, because they're obviously rubbish at this.



  • @rstinejr said:

    so I wrote an entry in our bug tracking system about a guaranteed NPE and went on.

    The colleague, instead of being slightly chagrined and removing the cruft, has lowered the priority from "Normal" to "Low".


    You put this in as "Normal"? As in, it's Normal for your program to throw an untrapped exception? Clearly this code is broken; and the colleague should be educated about his error; then forced to fix it to close the ticket. It should have been entered in as a blocker. It's clearly broken code; easy to fix, and the fix has no chance of introducing another bug/regression. Further, the colleague will understand the error and not introduce it in a really critical section of code later.



  • @DrPepper said:

    @rstinejr said:
    so I wrote an entry in our bug tracking system about a guaranteed NPE and went on.

    The colleague, instead of being slightly chagrined and removing the cruft, has lowered the priority from "Normal" to "Low".


    You put this in as "Normal"? As in, it's Normal for your program to throw an untrapped exception? Clearly this code is broken; and the colleague should be educated about his error; then forced to fix it to close the ticket. It should have been entered in as a blocker. It's clearly broken code; easy to fix, and the fix has no chance of introducing another bug/regression. Further, the colleague will understand the error and not introduce it in a really critical section of code later.

    If getKaka never returns null, is it a critical bug that the program throws an NRE when getKaka returns null?



  • @immibis said:

    @DrPepper said:
    @rstinejr said:
    so I wrote an entry in our bug tracking system about a guaranteed NPE and went on.

    The colleague, instead of being slightly chagrined and removing the cruft, has lowered the priority from "Normal" to "Low".


    You put this in as "Normal"? As in, it's Normal for your program to throw an untrapped exception? Clearly this code is broken; and the colleague should be educated about his error; then forced to fix it to close the ticket. It should have been entered in as a blocker. It's clearly broken code; easy to fix, and the fix has no chance of introducing another bug/regression. Further, the colleague will understand the error and not introduce it in a really critical section of code later.

    If getKaka never returns null, is it a critical bug that the program throws an NRE when getKaka returns null?

    if (false)
    {
    // any kind of cruft I want to put into the code
    }

    Yeah, it's a critical bug. It's broken code, plain and simple. You'd never write ((object)null).Dispose(). And, you can't guarantee the behavior of getKaka() can you? It sure looks to me like the original coder expected that getKaka could return null.


  • @DrPepper said:


    Yeah, it's a critical bug. It's broken code, plain and simple.

    Broken code doesn't work. The OP says it's a "guaranteed NPE" but that is only the case if the function returns null. We don't have the definition for that function so we don't even know if that is possible.IMO a "critical" bug is when trying to print a report corrupts the database or when trying to view orders adds a pair of yellow pants to the inventory table. By your logic [i]every[/i] software fault is "critical" which means that the categories are completely useless to begin with.

    You'd never write ((object)null).Dispose().

    Primary reason being that won't compile. (the using clause shows the sample is C#, and not Java, fwiw).

     

    And, you can't guarantee the behavior of getKaka() can you?

    You cannot guarantee the behaviour of calling methods either; it may be that they have exception handlers that deal with this.

     

    It sure looks to me like the original coder expected that getKaka could return null.
     

    I'm not sure I'd trust the judgement of somebody that has a piece of logic that works with an object only when that object is guaranteed to cause an NPE.

    I'm not sure why the code doesn't just wrap the method call in a using block. No null checks required and it would be shorter.

     

    A bug? yes. a Critical bug? No. Critical bugs are showstopper bugs that affect the customer, not code quality issues like this.

     


  • Discourse touched me in a no-no place

    So did you change it to an assertion (that kaka is never null) yet? That would be equivalent, and more informative too.



  • @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


  • Considered Harmful

    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.



  • @joe.edwards said:

    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.

    Jow, I agree 100% with your post...but that changes nothing about my post that there is a "chance" of the fix having a side-effect.



  • @TheCPUWizard said:

    @joe.edwards said:
    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.

    Jow, I agree 100% with your post...but that changes nothing about my post that there is a "chance" of the fix having a side-effect.


    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.


  • Considered Harmful

    @Ben L. said:

    @TheCPUWizard said:
    @joe.edwards said:
    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.

    Jow, I agree 100% with your post...but that changes nothing about my post that there is a "chance" of the fix having a side-effect.


    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.

    Like when an HttpWebRequest throws an exception for a response status code not between 200 and 399 inclusive (in a situation like a search spider where URLs may or may not be found)? Some exceptions aren't that exceptional, it depends on the situation. Though to be fair I think the framework should let the app decide if that's exceptional.



  • @joe.edwards said:

    @Ben L. said:
    @TheCPUWizard said:
    @joe.edwards said:
    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.

    Jow, I agree 100% with your post...but that changes nothing about my post that there is a "chance" of the fix having a side-effect.


    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.

    Like when an HttpWebRequest throws an exception for a response status code not between 200 and 399 inclusive (in a situation like a search spider where URLs may or may not be found)? Some exceptions aren't that exceptional, it depends on the situation. Though to be fair I think the framework should let the app decide if that's exceptional.
    What kind of language runtime has a HTTP client in it?


  • Considered Harmful

    @Ben L. said:

    @joe.edwards said:
    @Ben L. said:
    @TheCPUWizard said:
    @joe.edwards said:
    @TheCPUWizard said:

    @DrPepper said:

    ... and the fix has no chance of introducing another bug/regression.
     

    WRONG.... There is always the possibility that the method DOES return null, that an NRE is thrown, and the client code has a catch block to take specific action.


    I'd characterize behavior depending on a NRE to be inherently wrong. If you must be bug-compatible, just throw a NRE explicitly rather than dereference a known null reference. That's like writing 1/0 to throw an ArithmeticException - just stupid.

    Jow, I agree 100% with your post...but that changes nothing about my post that there is a "chance" of the fix having a side-effect.


    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.

    Like when an HttpWebRequest throws an exception for a response status code not between 200 and 399 inclusive (in a situation like a search spider where URLs may or may not be found)? Some exceptions aren't that exceptional, it depends on the situation. Though to be fair I think the framework should let the app decide if that's exceptional.
    What kind of language runtime has a HTTP client in it?

    Eh, the .NET Framework and CLR are basically inseparable, but I see what you're getting at.



  • @joe.edwards said:

    Like when an HttpWebRequest throws an exception for a response status code not between 200 and 399 inclusive (in a situation like a search spider where URLs may or may not be found)?

    HttpWebRequest is written bad and wrong, and I have no idea how it got put in the framework in its current form.



  • @blakeyrat said:

    @joe.edwards said:
    Like when an HttpWebRequest throws an exception for a response status code not between 200 and 399 inclusive (in a situation like a search spider where URLs may or may not be found)?

    HttpWebRequest is written bad and wrong, and I have no idea how it got put in the framework in its current form.

     

    What's wrong with it?  I like that it throws an exception if you get a non-success code.  Maybe it could use a TrySend or whatever.



  • @Sutherlands said:

    What's wrong with it? I like that it throws an exception if you get a non-success code.  Maybe it could use a TrySend or whatever.

    Because 404 isn't a "exception" to normal webserver behavior; it's expected to happen occasionally. It's a perfectly valid, normal, response. The HttpWebResponse class already has a place to plug-in the status, and can "null" the response stream. So why the exception? It makes no sense.



  • @blakeyrat said:

    @Sutherlands said:
    What's wrong with it? I like that it throws an exception if you get a non-success code.  Maybe it could use a TrySend or whatever.

    Because 404 isn't a "exception" to normal webserver behavior; it's expected to happen occasionally. It's a perfectly valid, normal, response. The HttpWebResponse class already has a place to plug-in the status, and can "null" the response stream. So why the exception? It makes no sense.

    404 responses still have responses. I'd hate to see a web browser that refuses to display 404 pages!


  • Considered Harmful

    You can read from the error stream, but you have to catch the exception first. And yeah, any time you can't achieve a typical use-case without catching an exception is poor design.


  • Discourse touched me in a no-no place

    @Ben L. said:

    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.



  • @dkf said:

    @Ben L. said:
    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.



  • @Ronald said:

    @dkf said:
    @Ben L. said:
    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.



  • @Ronald said:

    @dkf said:
    @Ben L. said:
    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.

    Then the server will return 500 Internal Server Error and annoy Blakey.



  • @Ben L. said:

    @Ronald said:
    @dkf said:
    @Ben L. said:
    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.

    It would be awesome if the second option (type without help) meant that clippy would type the letter without the user help. This would also make this picture relevant to the discussion.



  • @Zemm said:

    @Ronald said:
    @dkf said:
    @Ben L. said:
    General rule: If your exception/error/panic is thrown by the runtime, DO NOT CATCH IT. Fix it instead.
    General rule: only catch errors when you can do something about them. And don't listen to Ben L.

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.

    Then the server will return 500 Internal Server Error and annoy Blakey.

    That's why God created error pages. And they are easy to do, just one line of code:

    history.back();
    

  • Discourse touched me in a no-no place

    @Ronald said:

    Perl has the right solution. Forget about errors, let the machine do what it thinks you would have wanted to do in the event of an unexpected situation. It's called empowerment.
    Perl basically operates on the theory that everything is a short-running script. It's the polar opposite of Java (where everything appears to be a long running server). Drag a language out of its comfort zone and it will end up sucking.

    Well, sucking worse than normal.



  • Rather than argue about whether getKaka() might return null or not, I'd think the more serious bug is that because of the typo, kaka is likely not being Dispose()'d at all.


Log in to reply