Zorba

Use of "catch (...)" should be reduced or mitigated

Bug #1103819 reported by Paul J. Lucas
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Zorba
In Progress
High
Paul J. Lucas
Zorba 3.0 "Kratos"

Bug Description

At various places in the code, the "catch" clause of a try/catch is "catch (...)". In many cases, this isn't handled as well as it ought to be. For example:

  catch ( ZorbaException const &e ) {
    // do something
  }
  catch ( ... ) {
    // do something more drastic
  }

At the very least, an additional "catch" clause should be inserted like:

  catch ( ZorbaException const &e ) {
    // do something
  }
  catch ( std::exception const &e ) {
    // do something with e.what()
  }
  catch ( ... ) {
    // do something more drastic
  }

so that at least the information contained in the "what()" message can be preserved.

Another case is in loader_fast.cpp, FastXmlLoader::loadXml() that does a "catch (...)" and returns NULL. It's not clear this is the right thing to do. Perhaps it ought to clean-up then rethrow the exception. (This case also ought to catch std::exception for the same reason given above.)

Related branches

lp:~zorba-coders/zorba/bug-1103819
Ready for review for merging into lp:zorba
Chris Hillery: Needs Information
Paul J. Lucas: Approve
Diff: 1283 lines (+397/-393)
16 files modified
src/api/xqueryimpl.cpp (+3/-17)
src/capi/cdynamic_context.cpp (+1/-4)
src/capi/cexpression.cpp (+1/-4)
src/capi/cimplementation.cpp (+1/-4)
src/capi/csequence.cpp (+1/-4)
src/capi/cstatic_context.cpp (+1/-4)
src/context/default_url_resolvers.cpp (+6/-0)
src/runtime/debug/debug_iterator_impl.cpp (+11/-18)
src/runtime/indexing/doc_indexer.cpp (+1/-3)
src/store/naive/loader_dtd.cpp (+118/-8)
src/store/naive/loader_fast.cpp (+51/-89)
src/store/naive/node_items.cpp (+13/-27)
src/store/naive/node_updates.cpp (+179/-186)
src/store/naive/pul_primitives.cpp (+5/-1)
src/store/naive/store.cpp (+2/-3)
src/types/schema/schema.cpp (+3/-21)
lp:~zorba-coders/zorba/bug1103819-catchAll
Rejected for merging into lp:zorba
Cezar Andrei: Approve
Paul J. Lucas: Pending requested
Diff: 287 lines (+131/-2)
2 files modified
src/store/naive/loader_dtd.cpp (+69/-0)
src/store/naive/loader_fast.cpp (+62/-2)
Paul J. Lucas (paul-lucas)
Changed in zorba:
status: New → In Progress
Paul J. Lucas (paul-lucas)
Changed in zorba:
milestone: none → 2.9
Dana Florescu (dflorescu)
Changed in zorba:
importance: Undecided → Medium
Revision history for this message
Chris Hillery (ceejatec) wrote :

Cezar, can you take a look at the FastXmlLoader issue Paul mentions?

Changed in zorba:
assignee: Paul J. Lucas (paul-lucas) → Cezar Andrei (cezar-andrei)
Revision history for this message
Paul J. Lucas (paul-lucas) wrote :

Why are there two branches for this?

Revision history for this message
Cezar Andrei (cezar-andrei) wrote :

Why not? I fixed the catches in loader_fast and loader_dtd, pushed them to a branch and discovered there was another branch. Is there any problem if there are 2 branches, looks like launchpad supports it?

Cezar Andrei (cezar-andrei)
Changed in zorba:
assignee: Cezar Andrei (cezar-andrei) → Paul J. Lucas (paul-lucas)
Revision history for this message
Paul J. Lucas (paul-lucas) wrote :

The file loader_dtd.cpp still isn't finished. See the "TODO" comment in the file.

Changed in zorba:
assignee: Paul J. Lucas (paul-lucas) → Cezar Andrei (cezar-andrei)
Revision history for this message
Paul J. Lucas (paul-lucas) wrote :

BTW: branch bug1103819-catchAll was merged into the bug-1103819 branch so now there is only one branch. Please use the bug-1103819 branch only.

Revision history for this message
Cezar Andrei (cezar-andrei) wrote :

I've put in bug-1103819 branch two fixes. Comments?

Revision history for this message
Paul J. Lucas (paul-lucas) wrote :

For lines like those starting at 762 in the diff, why bother to catch the exceptions if all that happens is calling ZORBA_FATAL? You simply could not catch any exception and let it propagate upwards through the call stack.

Revision history for this message
Cezar Andrei (cezar-andrei) wrote :

Not sure what file are you talking about but if it's src/store/naive/node_items.cpp, bzr blame shows you and daniel working on those.

It's not exactly the same, "Unexpected exception" gives a little more info on the matter. I don't know the code, to say if the message can be made more useful.

Cezar Andrei (cezar-andrei)
Changed in zorba:
assignee: Cezar Andrei (cezar-andrei) → Paul J. Lucas (paul-lucas)
Revision history for this message
Paul J. Lucas (paul-lucas) wrote :

I mean if you go to this page:

  https://code.launchpad.net/~zorba-coders/zorba/bug-1103819/+merge/146312

and look at the "Preview Diff" you will see that each line of the diff is numbered. If you scroll down until you see "762," that's what I'm talking about.

Those lines were apparently touched by Daniel, not me.

If you don't catch the exception at all, it will "bubble up" and eventually be caught by some other code that will also print "unexpected exception," so there's no point in catching it here.

Changed in zorba:
assignee: Paul J. Lucas (paul-lucas) → Cezar Andrei (cezar-andrei)
Cezar Andrei (cezar-andrei)
Changed in zorba:
assignee: Cezar Andrei (cezar-andrei) → Paul J. Lucas (paul-lucas)
Chris Hillery (ceejatec)
Changed in zorba:
milestone: 2.9 → 3.0
Chris Hillery (ceejatec)
Changed in zorba:
importance: Medium → High
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.