Skip to content
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

[API] Clarify expectations for C++ exceptions, and usage of noexcept methods. #3013

Open
marcalff opened this issue Jul 26, 2024 · 5 comments
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@marcalff
Copy link
Member

Several methods in the API are flagged as noexcept.

This is desirable, because adding instrumentation to an application (i.e., calling opentelemetry-cpp apis) should not make the application less stable.

In particular, any failure in the opentelemetry-cpp sdk or exporters should not propagate the exception up, taking the application down.

To comply with the noexcept contract, methods in the SDK implementation should never raise exceptions.

According to clang-tidy reports, this is not always the case.

This part should be revisited, to clarify expectations, and enforce the SDK implementation complies.

cc @msiddhu

@marcalff marcalff added the bug Something isn't working label Jul 26, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 26, 2024
@marcalff
Copy link
Member Author

@msiddhu
Copy link
Contributor

msiddhu commented Jul 29, 2024

I'll work on this. Currently, there are 20 warnings flagged by clang-tidy as they may raise errors. I'll make a sheet explaining why and will come up with a cleanup. I'm busy this week but will raise a PR by late next week.

@esigo esigo added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 7, 2024
Copy link

github-actions bot commented Oct 9, 2024

This issue was marked as stale due to lack of activity.

@github-actions github-actions bot added the Stale label Oct 9, 2024
@Andrew-Brock
Copy link

Hi @msiddhu, did you get anywhere in generating the spreadsheet for this? I'm interested in the status of it because in the current state, the functions being marked as noexcept ironically make the API more likely to introduce instability due to it now calling std::terminate rather than giving me a chance to handle the exception, especially in the case of the Observable metrics in sdk::metrics::ObserverResultT::Observe where the exception would be off in its own thread.

I also see that in places where this is attempted to be handled, such as in #3012, that the exception handling can also throw exceptions such as when building up the std::stringstream tmp_stream or in the registered LogHandler::Handle, as-per the definition of the OTEL_INTERNAL_LOG_DISPATCH macro.

I'm happy to submit some small PRs to address some but your comment hints that some of these may not be so trivial to fix.

@github-actions github-actions bot removed the Stale label Jan 4, 2025
@Andrew-Brock
Copy link

I've come across another case where exceptions may be thrown in non-obvious ways in functions marked as noexcept.

Some places constructing a shared_ptr where the object is allocated with new (std::nothrow). In this situation if the allocation fails the pointer will be nullptr, but the very next thing to happen will be the shared_ptr performing another allocation internally for the control block. If the previous allocation failed then there's a good chance that the 2nd allocation will fail too, but this time throwing a std::bad_alloc.

There are several places that do this. 1 example is in Tracer::StartSpan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

4 participants