Task #2997
closedEnhance exception throwing with Boost Exception library
100%
Description
Replace all throw
exceptions with BOOST_THROW_EXCEPTION
macros.
This annotates the exception with diagnostic information, so that an application using ndn-cxx library can access them for easier debugging.
Updated by Junxiao Shi over 9 years ago
- Related to Task #2541: Enhance exception throwing and fatal log with Boost Exception library added
Updated by Spyros Mastorakis over 9 years ago
- Status changed from New to In Progress
- Assignee set to Spyros Mastorakis
- % Done changed from 0 to 10
I am uploading an initial commit in gerrit with a single Error statement changed. Please let me know if you like this structure or I should change it:
Updated by Alex Afanasyev over 9 years ago
I haven't yet seen the diagnostic message output and don't remember how it looked like. But I think it would be rather verbose and not entirely user friendly. Should we enable it only in certain circumstance (like when compiled in debug mode)?
Updated by Davide Pesavento over 9 years ago
Crashing is never user friendly anyway. But if it's a real bug and the user opens a ticket with the extended error message, it'll be easier for us, and better than asking to (re)build in debug mode and try to reproduce the crash. So I think we should always print the extended info.
Updated by Spyros Mastorakis over 9 years ago
- Status changed from In Progress to Code review
- % Done changed from 10 to 90
Updated by Junxiao Shi over 9 years ago
There is a dispute on whether to print diagnostic information in ndnsec
top-level catch
.
ndnsec
top-level catch
is serving dual purposes:
- Catch and print a legitimate error, such as "Key not found". In this case, it's better to print the error message
e.what()
only without diagnostics. - Catch unexpected error, such as TLV decoding errors. In this case, diagnostics are useful.
We should distinguish between these two purposes (using two or more separate catch
branches with different types), and skip diagnostics for all legitimate errors.
Updated by Davide Pesavento over 9 years ago
Junxiao Shi wrote:
ndnsec
top-levelcatch
is serving dual purposes:
- Catch and print a legitimate error, such as "Key not found". In this case, it's better to print the error message
e.what()
only without diagnostics.
This is abusing exceptions. "key not found" is not an exceptional case. In such case the program knows exactly what's wrong and how to solve it (exit with a clear error for the user). So you simply shouldn't use exceptions for this case.
- Catch unexpected error, such as TLV decoding errors. In this case, diagnostics are useful.
We should distinguish between these two purposes (using two or more separate
catch
branches with different types), and skip diagnostics for all legitimate errors.
Don't use exceptions for non-exceptional cases.
Updated by Junxiao Shi over 9 years ago
I agree with note-7.
I recommend this to proceed as follows:
- Don't add diagnostics in
ndnsec
. - Create a separate issue to avoid exceptions in non-exceptional cases;
ndnsec
top-levelcatch
should add diagnostics in that issue.
Updated by Spyros Mastorakis over 9 years ago
Is there a consensus on how we should proceed with this issue?
Updated by Alex Afanasyev over 9 years ago
I agree with note 8 only partially. I would revert changes in ndnsec tools, but I don't agree with other changes. Exception that are throws from security classes are exceptional cases and need to be reported to users. ndnsec tools are user facing tools and users just need a high-level error (we may need to update error messages to make errors more user friendly in some cases).
We can also revert any changes in other tools, as they will be removed from the library anyways.
For NFD (I think it should be just nfd, not other tools), we need to agree on a reasonable format of the error message. I would like to see just content of what(), file name and line number of the place the error was thrown (if available).
Updated by Junxiao Shi over 9 years ago
- Status changed from Code review to Closed
- % Done changed from 90 to 100