need argument for try catch blocks

Discussion in 'C++' started by Christopher, May 26, 2011.

  1. Christopher

    Christopher Guest

    I've got my self an old c style programmer reviewing my code at work.

    I've already discovered a plethora of bugs in that exceptions are
    getting thrown from native code, the windows API, and our own code and
    never handled.

    However, in my code reviews, I've got items about "unecessary try
    catch blocks." I really don't know why it is even worthy of putting in
    a code review. Maybe he is thinking efficiency or readability?

    My point of view is that when your dealing with a huge project with
    poor documentation, it is better to always surround anything that
    throws with a try catch block, even if it seems unecessary. I can't
    count the number of hours I've spend debugging code that was bugged
    because of no error checking. The code changes all the time and a
    catch that might never get hit, could get hit after many edits. If
    nothing else it seems like a good way to assist the poor guy
    debugging. It says to me, "Hey an error could occur here!"

    My problem is in convincing my peers. I cannot formalize my argument.
    Is there an source with clout that will back my argument up? Or
    perhaps someone can put into better words what I am describing? Or am
    I just wrong?
     
    Christopher, May 26, 2011
    #1
    1. Advertisements

  2. Christopher

    Balog Pal Guest

    Being C programmer helps little using C++, it is more like a hindrance. To
    be good in C++ you shall unlearn very much.
    Unhandled exception is not a coding mistake but part of fundamental design.
    The project shall have an exception handling policy that makes it clear
    where the boundaries are and where to catch. Making stuff obvoius. And
    wothout no wonder you have chaos.
    Naturally. Catch supposed to be very rare. And restricted to library border
    and high level functions.
    Likely just know how code is supposed to be. The norm is to be
    exception-neutral. Possibly with some odd throws. If there is a catch, the
    content shall make it obvious that it is necessary. If there is doubt it
    shall not be there.
    Huh? Hodgepodge will not make anything better. Adding lines without a clear
    and sensible reason just increases the mess. For exceptions it is even
    more so, as they propagate finely. So the only code that catch is the one
    that knows too well what it expects and what to do about it. And why,
    obviously.
    Exceptions make it quite impossible to "not check" in the C sense, so your
    case can not apply. The most that can happen the program is hit with
    exception no one thought to handle, where you get terminate() as the best
    default.

    But to have anything better you shall be able to show the point between
    emitter and main() where it was supposed to do anything else to call it a
    coding bug. Just inserting random try blocks at random places is not that.
    Care to explain how it says that at the random catch site? And how you can
    state it is "error", when upstream someone could have the proper catch?
    No wonder.
    Don't formalize then. Just try to make your case, say here. In a real sense.
    Show some "broken" code and your "fixed" version. Explain the difference,
    showing the situations when it is better. Then be ready to defend your
    position. :)
    We will see as soon as "argument" is actually presented.
    Most likely the latter, but do make your case, so can learn either this or
    that way.
     
    Balog Pal, May 26, 2011
    #2
    1. Advertisements

  3. Christopher

    Christopher Guest


    Well, I don't really view it as "random" catch site. I put a catch on
    anything that throws.

    A few examples:

    1) A dll that contains multithreaded code that is part of a window's
    service contains a method that uses boost::lexical_cast on a string
    that came in as a parameter.

    I'd surround the lexical cast with a try-catch. If boost threw, I
    create an error message complaining that the parameter contained
    invalid text, log it, and throw a (company made) invalid argument
    exception. Supposedly, the code higher up will catch this and
    determine if the action it was trying to take requires program
    termination, more logging, or what to do.

    2) Same scenario except I am casting a static constant integer to a
    string.
    My argument is that later on that static constant integer may no
    longer be. The design is changing, things get edited all the time, and
    I don't see the harm in providing some clarity for the next guy that
    "hey boost can throw here!", rather than leaving them to look up every
    single function call in various referenced and checking if they throw
    or not.

    3) A constructor of an object makes a connection to a database. If the
    database connection fails, the object cannot be constructed. Thus, I
    throw an exception with a "DB connection failure" error. I then catch
    it in the method that tryed to construct the object. He would argue to
    let the exception travel on up. I'd argue that it would be better to
    catch it there to let other programmers know that construction can
    fail
    due to a database connection error.


    A huge problem in this project is that somebody at some time had the
    mentality of "if any error occurs anywhere, throw an exception and
    don't worry about releasing resources, catching and handling, or
    service termination." They put some 3rd party library at the top that
    catches any unhandled exceptions and logs them. But they didn't seem
    to care about the undefined state it left the system in!

    I am trying to systematically fix that as I go throw each source file.
    To do it at once seems an impossible task. I think a beginning is to
    start writing try/catch blocks on a case for case basis and determine
    how to handle the error conditions one at a time. I'd rather
    arbitralilly catch, than arbitrallily let an exception go up the stack
    and take it for granted that resources were released and someone else
    is catching every flavor of exception and handling it!

    However, it seems you might have a different view in that an exception
    should be allowed to travel up the stack in some cases where it needs
    to be well-defined in the design where they should be handled. We
    don't have any such design in place. So, what does a lowly minion with
    barely any input on design, do in that case?
     
    Christopher, May 26, 2011
    #3
  4. Christopher

    Christopher Guest

    Wow do I have spelling errors. I wish you could edit newsgroup
    posts :p
    Sorry, I hope it still makes sense. I can provide clarity if it is
    needed.
     
    Christopher, May 26, 2011
    #4
  5. Christopher

    Balog Pal Guest

    Bad Thing (TM).

    Error codes are for communication with the immediate caller. Exceptions are
    for communication long distance. Several callers upward typically. It is
    like checking passports on every corner instead of doing that only at the
    country borders.
    Converting exceptions to others is fair in itself. It may be good too, if
    you have that single use of the boost facility. However if your DLL uses
    boost profoundly, you better use try blocks on your dllexport functions. And
    leave the whole implementation alone.
    The more the implementation changes the less you want to maintain the
    avalanche. Exception are, uh, exceptional. (provided a correct design).
    Normally when you're hit by one, you can't do much but cancel a full
    high-level operation (or the whole process). Consequently it is enough to
    handle them at those few locations. Collectively. And in a simple, stable
    way.

    As i said, converting from one exception to another is fair, but consider if
    there is a benefit really.
    Logging is a generic excuse, but IME is way less helpful than claimed. The
    natural point is again at the top level, you state the imput params,
    possibly the environment and the content of the caught exception. If you
    skip that, and only log at a lower point, you'll know what happened but not
    why. While having both is mostly redundant.
    Sure. ctors are common exception sources.
    Why on Earth?
    Definitely.

    Is DB opening expected? So let the function expect it, and just do its job.
    The module dealing with DB can emit arbitrary amount of exceptions,
    practically anywhere. If nothing else, SQL timeout is beyond your control,
    and can happen in any (otherwise correct) operation. And you have no
    realistic way to recover at low level. While on the high level youre not
    interested at all which of the 1298 operation failed.
    Heh? If you want to teach programmers, do it in the proper institution.
    Catching exceptions are not done for teaching puurposes. the only purpose to
    catch the exception is to do some relevant action.

    In your example action WHAT do you do in the catch block?
    Certainly all your functions must bring at least the "basic guarantee" from
    the Abrahams classifications.
    If that holds, the statement is right: you shall not worry about anything.
    Resources are handled by RAII. Exceptions are caught upstream where makes
    sense. Your job is to stay out of the way, and do the actual work.
    Maybe we're getting somewhere. If the program is in undefned state that is a
    problem indeed. But instead of infesting the program with catch clause, you
    shall concentrate on that alone: reviwe and reorganize the functions that
    are not exception safe.

    Hint: it is NOT done using try blocks. But by using RAII, the swap idiom
    and stuff like that. mainly by avoiding to mess up the state in the first
    place, and break invariants.
    I'm still not convinced your fix counts as one. I saw such attempts and
    most only pushed the problem and made it subtler, instead of eliminating the
    roots. did you ate least some fundamental literatire on exception safety,
    and how to reach it in C++? I recently suggested someone to check out
    gotw.ca.
    No, the beginning is to learn the fundamental C++ idioms. Aim, set, fire.
    Not the other way around.
    If you have non-controlled resources around your fight is futile. And if you
    want to fix the situation start RAII-zing the stuff as first step.
    In a healthily organized code you can actually do as your quote above:
    return or throw at any (almost) point of the code, and have sensible
    outcome. And if the code is not like that, you're doomed anyway.
    So another starting point is to have it. Actually THAT is the thing that
    is supposed to teach and guide the programmers. :)
    Do you really want my suggestion? In such a situation I would either fix it
    in the real sense, or quit for good. Actually did a plenty of both.
     
    Balog Pal, May 26, 2011
    #5
  6. In general try/catch blocks do not make the code any slower, so an
    efficiency argument is unjustified.
    If an exception is thrown, it will be caught somewhere (or thrown out of
    main(), which is less than ideal). Any good debugger will tell you where
    the exception was thrown from (and the stack trace).
     
    Juha Nieminen, May 27, 2011
    #6
  7. Christopher

    Stefan Ram Guest

    Not »handling« an exception does not have to be a bug.
    Who? One uses the personal pronoun »he« after a person has
    been introduced.

    So, if you already know »him«, why can't you ask him?
    This is exacly what exceptions were invented to prevent.
    Exceptions already alter the control flow, so that an
    explicit »error check« often is not necessary.

    Is this application code or library code?

    What is your idea of what to do in the catch block?
    I suggest that you read the chapters on exceptions in some
    text books.
     
    Stefan Ram, May 27, 2011
    #7
  8. Christopher

    Edek Guest

    What do those blocks do when they catch something?

    If they do anything that is necessary or of value, they should be there.
    Naturally, the project formal/informal policy is one thing to follow,
    but in general such 'extra' try-catches are done for example:

    a) to clean up resources (like DB, if destructors are not enough), then
    they are necessary
    b) to rethrow as another exception - utility really, see d)
    c) to log, if there are no stack traces and for some reason a debugger
    cannot be used, or just it is quicker to have a log without plugging in
    a debugger
    d) to work around caller code, which does not really handle exceptions,
    or handles just a few types - then catching is better than terminate()
    Efficiency? No. Readability? It depends, mainly on what the reader is
    used to. Different people, different habits.
    I find that very true. Even if no exception can be throw now, some time
    from now it may be thrown because of callee changes, so to be on a safe
    side, it is good to try/catch. Good maintained code is usually handling
    errors nicely 'by design', no matter when the faults are introduced.
    HTH
    Edek
     
    Edek, May 27, 2011
    #8
  9. Christopher

    Balog Pal Guest

    Hell no. Resources must be handler by RAII, and never left to hang around
    "raw". And mandatory release code must not be multiplicated at every
    possible exit.
    Logging is a suspicious reason, and if really needed also can be done better
    in a RAII-like form. Though special cases may apply.
    I'd rather call it "when exceptions must be converted to error return codes
    for the caller".
     
    Balog Pal, May 27, 2011
    #9
  10. Christopher

    Edek Guest

    Is it possible to do anything which may throw during stack unwind? I
    always thought that was UB.
    Like having no exception stack trace? Hell, I like getting at the top of
    the thread an information along the lines of 'Integer conversion failed'
    and nothing more.
    Sure.
     
    Edek, May 27, 2011
    #10
  11. That's a good analogy. Life Would Suck (TM) if passports were
    checked on every corner. And code that checks every statement
    /individually/ for exceptions Sucks For Sure.

    KHD
     
    Keith H Duggar, May 27, 2011
    #11
  12. Christopher

    Balog Pal Guest

    Depends. If you use the standard library, it states that you'll have UB if
    your type T emits an exception from a destructor.

    In general that is not UB, but a double exception will end up in
    terminate(). And a throwing destructor can leave behind memory leaks,
    incosistencies and much other stuff you do not want. for that reason it is
    a strong guideline that destructors must be nothrow. Those ignoring it are
    likely in peril regardless other stuff.

    If you write correct software, you will not have that. If it is caused by
    not software but input external input, then you can dump the input at that
    place, and figure out why it was not fit for purpose.

    As I told before, chasing more log and traci is a usual activity that
    correlates tith WTF-ish codebase. The rrot fix of that is not adding more
    trace support but start work on reviews and fixes.

    Whily really hairy environments support externally controllable trace. Like
    the linux kernel -- you can echo > to turn on tracepoints, debug messages on
    a single module, or even on a single line of source if interested. And it is
    NOT there for the regular case. Neither needs excess code for tracing (only
    for special messages).

    And adding noise to the code provably hinders making it right. While making
    it simpler and easier to read leads to less problems to look after. Or
    putting the same effort into unit tests will more likely find the cause of
    your integer conversion.
     
    Balog Pal, May 27, 2011
    #12
  13. Christopher

    Edek Guest

    Yes, that I know. Actually, what I mean is this

    struct Deleter {
    ... holds a DB connection

    ~Deleter () {
    try {
    .. whatever that may throw
    } catch (...) {} // does not emit. But is it legal?
    }

    int some {
    scoped_ptr<Deleter> del(new Deleter(openDbConnection()) );

    // ... here something throws
    // destructor throws and catches
    // is this UB?
    }

    Is this UB or not? I've always thought it is.
    Ok, integer conversion is just an example, could be anything.
    Yes, like entering an important function pushes something on TLS log
    context, or whatever via RAII. I know. Somewhere in this thread there
    was something said about hard debugging.
    Sure, there is a fake tradeoff between quality and effort. Quality here
    would mean that there is no need (or place) for weird stuff, and
    debugging would be easy. That's when a project has a reasonable policy,
    in this case of error handling, even in the midst of various libraries.
    The tradeoff works in the short run, in the long run is a pain, I'm not
    sure about the overrated TCD ;) (Total Cost of Development).

    But this is more about the project in large. I do prefer raii, but it
    needs consistency.

    Edek
     
    Edek, May 27, 2011
    #13
  14. Christopher

    Balog Pal Guest

    Yes, that is perfectly legal. The standard tays that if a dtor called in the
    stach unwinding *exits* through an exception, then terminate() is called. So
    you are supposed to catch and swallow them. (see section 15.2 or .3)

    Some libraries use uncaught_exception() in dtors and only swallow if it is
    true. IIRC that is not a surefire way and the return value is not in exact
    match with that intent, but I forgot the details. But as alrady mentoned,
    emitting dtor has problems in other contexts too, so the above code makes
    general sense, if anything not nothrow() is actually called.

    The only realistic bad case I recall if for CFile used as output and not .
    Close()-d in MFC 4.2 (might got fixed for more current versions).
    With the try-and swallow it is okay.

    for the standard library the applicable part is at 17.4.3.6 bullet 4
    In particular, the effects are undefined in the following cases: ...

    - if any replacement function or handler function or destructor operation
    throws an exception, unless specifically allowed in the applicable Required
    behavior paragraph.

    This only applies to exceptions actually escaping.
     
    Balog Pal, May 27, 2011
    #14
  15. Christopher

    Christopher Guest

    I'd love to be counting on destructors. So far though, I'd say 60% of
    the code is written in C-style, with global pointers being to things
    allocated in various class methods, released in others, global
    instances being used left and right, macros, etc. etc. Things are just
    not well contained. There are classes where the functionality is
    partly handled by global functions and partly handled by class
    methods. But, I am ranting.

    My point is though, out here, at least in my city and for several jobs
    now, there always seems to be that c-style programmer on the team that
    does this kind of thing ruining or at least making it difficult to
    follow most of the OO principles. They are usually well respected with
    seniority too. Things get into such a mess that I don't know where to
    start fixing it. I wish I could just mind control someone and convince
    them to follow S.O.L.I.D principles, RAII, and several other things.

    Anyway, the advice in the posts is good. You guys have changed my mind
    on exception handling. Maybe I will try to convice my team that we
    should take the time to come up with a design decision on when and how
    to handle exceptions, their types, identify these boundries, etc.
     
    Christopher, May 27, 2011
    #15
  16. Christopher

    Stefan Ram Guest

    One can always call anything from within a destructor. One
    then just has to wrap everything with »try« and »catch«, so
    that exceptions /do not leave/ the destructor.
     
    Stefan Ram, May 27, 2011
    #16
  17. Christopher

    Balog Pal Guest

    Indeed. What you say is all the MORE reason to start reshaping code so that
    all resources have an assigned handler. You have NO chance to get rid of
    leaks otherwise. Only to shift them around and make subtler. Dragging in a
    swamp of code in the meantime that could be not there at all.
    Yes, it is common. As we know like 90% of software products are crapware,
    and still like 50% fails to deliver at all. Life is hard. It will be even
    harder if you join the bandwagon and ADD to problems instead of fight to
    reduce them. If you fail in tha latter, at least can look in the mirror
    and say that you did try to do the right thing. And eventually may hit a
    place where you succeed. As opposed to keep working in the latrine
    endlessly.
    Yep, as incompetene start at managers, and "respect" too often means "suck
    up" instead of any original meanings. While "seniority" measured in years
    passed instead of working stuff created.

    Sorry, can't tell you anything bright. OTOH you're your own life's master,
    and *can* shape what happens with you.
    First learn who can be convinvced. Many people are locked in their ideas
    and will not budge. Even by showing working stuff. (loop up NIH). Do not
    waste time on them, but go around. While those who can be convinced
    eventually will.
    You have much better chance if you write a policy yourself in a shape that
    works, then push it through, possibly with some tweaking. Waiting for
    non-interested parties to form a committee and a committee to create
    anything useful is just recipe for stalling.
     
    Balog Pal, May 27, 2011
    #17
  18. Christopher

    Edek Guest

    If 60% of code is C (or C-style), I wouldn't be orthodox about RAII.
    Either the developers do have ways to handle debugging and so on, maybe
    just slightly home-made, or maybe some ways can be gradually introduced.

    Edek
     
    Edek, May 27, 2011
    #18
  19. Christopher

    Fred Zwarts Guest

    This works, of course, only if the destructor knows how to handle the
    exception.
    But what should the destructor do with unknown exceptions?
    Ignoring them will cause the program to continue in a undefined state.
    Is that better then UB?

    So, just wrapping a destructor code in a

    try {
    ...
    catch (...) {}

    is not a real solution.

    For example, if memory is properly managed, a delete should not throw an
    exception.
    If it does, it may be an indication of memory corruption.
    Should the destructor ignore it and let the program continue,
    or is it better to let the exception leave the destructor, so that the
    program will terminate,
    or should it immediately call terminate and not give outer functions a
    chance for properly freeing resources?

    I am not so sure whether a destructor should never throw.
    My code handles only the exceptions I know to handle.
    It might be dangerous to catch unknown exceptions and ignore them.
     
    Fred Zwarts, May 30, 2011
    #19
  20. Christopher

    Balog Pal Guest

    What undefined state? Normally throw happens related to particular object.
    With basic guarantee the caller will no the object in question has not
    reached the known state. So mey do some action. For dtors, the object will
    be gone so that does not apply. Do not confuse unknown state with undefined
    behavior.
    Still better than throwing for the general case. As if that happens, the
    dtor chain is stopped right there. Is a member object's dtor throws the
    other members in the same complete objects will not be called. If dtor
    halfway in the array throws, dtors for the rest of the array are not called.
    So throwing you disrupt the symmetry and will cause leaks unless very lucky.
    If memory is detectably corrupt then you shall not throw but halt in tracks.
    Throw is NOT for handling program bugs but for recoverable runtime
    conditions.
    Is it so hard to design dtors to be nothrow? Why should it do anything that
    requires check and throw?

    The only dtor I recall that could throw was closing file, flushing buffer on
    the way that could fail. And leaked to dtor with calling the general Close()
    if the file was opened. But it is a sucking design ayway. When you do such
    stuff you shall call Close() explicitely and carry on if it survived. If
    nuking the object before that; for an implicit call it can just do the core
    work, release the resource and swallow. Those interested in completion did
    have a chance to force it earlier.
    google for expert articles, mentors wrote a plenty of stuff about that, and
    damn convincing too.
    In this case swallowing is the proper handling by default.
    So the design shall not rely on dtors throwing in the first place, then said
    danger is gone for good. Call every "commit" explicitly, while rollback and
    get-rid type operations shall just do the best effort.
     
    Balog Pal, May 30, 2011
    #20
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.