-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-31347 Jtrace Jlog::export should honor noexcept declaration #18340
HPCC-31347 Jtrace Jlog::export should honor noexcept declaration #18340
Conversation
https://track.hpccsystems.com/browse/HPCC-31347 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tackling the symptom rather than the cause. Looking closer, I think the problem is that printAttributes() calls UNIMPLEMENTED - which throws an exception. With this change that will silently be ignored (and leak the exception).
Better would be to change how that error is reported.
The commit message also contains a couple of typos.
hmmm we discussed the difficulty in determining all possible exceptions before I submitted this PR, maybe that doesn't apply here. Even though it might be targeting symptoms, preventing uncaught exceptions in a noexcept, non-critical function seems warranted. Do you disagree, and would prefer we attempt to tackle all possible exceptions individually? Also, unfortunately my eyes are not as sharp as yours and I was not able to spot the specific exception caught by coverity, nor the the UNIMPLEMENTED call you mentioned {btw github pilot couldn't identify any possible IExceptions for what that's worth) Also, my best interpretation of "Better would be to change how that error is reported." is vague (find the UNIMPLEMENTED line, and log, but don't throw?, catch StringExceptions log message release exception, return failure? catch StringException, release exception, return failure?, catch iexceptions?) |
7e5e3b3
to
2b8925b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment for suggested change.
I found the cause of the exception by reading the coverity report carefully - it narrowed down where the exception was thrown.
system/jlib/jtrace.cpp
Outdated
} | ||
return opentelemetry::sdk::common::ExportResult::kSuccess; | ||
} | ||
catch(...) //Interface does not allow throwing exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best code for this situation would be something like:
catch (IException * e);
{
EXCLOG(e, "JLogSpanExporter::Export");
e->Release();
return opentelemetry::sdk::common::ExportResult::kFailure;
}
This will ensure that the issue does get logged, and will cleanly catch and release the exception.
21bf908
to
4777031
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Please squash and I will merge
- Catches all IExceptions thrown in noexcept function - Logs issue - Releases exception - Reports export failure if exception caught Signed-off-by: Rodrigo Pastrana <[email protected]>
4777031
to
6491527
Compare
@ghalliday done |
Type of change:
Checklist:
Smoketest:
Testing: