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

Add exception guard to FFI boundary. #1212

Merged
merged 2 commits into from
Jul 22, 2020

Conversation

dewyatt
Copy link
Contributor

@dewyatt dewyatt commented Jul 21, 2020

This uses function try blocks to prevent C++ exceptions from propagating
across the FFI boundary. Some functions which return direct values
(rather than error codes) and do not throw are left as-is.

For now it just prints directly to stderr and uses a format similar to RNP_LOG, which I think is a good compromise considering that it should be a very rare occurrence.

Fixes #839

This uses function try blocks to prevent C++ exceptions from propagating
across the FFI boundary. Some functions which return direct values
(rather than error codes) and do not throw are left as-is.
Copy link
Contributor

@ronaldtse ronaldtse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent stuff. Thanks @dewyatt !

Copy link
Contributor

@antonsviridenko antonsviridenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be nice to add some tests that throw various exceptions and check that everything works as expected

@dewyatt dewyatt force-pushed the dewyatt-839-exceptions-ffi-boundary branch from b27cc72 to 56cd0ff Compare July 22, 2020 06:50
@dewyatt
Copy link
Contributor Author

dewyatt commented Jul 22, 2020

Maybe it would be nice to add some tests that throw various exceptions and check that everything works as expected

Added some tests. It only tests against a single FFI function really but it does test a few different exception types.

Copy link
Contributor

@ni4 ni4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks, @dewyatt. But let's wait for @antonsviridenko comments as he was first to review.

Copy link
Contributor

@antonsviridenko antonsviridenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ni4
Copy link
Contributor

ni4 commented Jul 22, 2020

@antonsviridenko Feel free to rebase and merge once there are two or more approvals.

@antonsviridenko antonsviridenko merged commit 0ca0249 into master Jul 22, 2020
@antonsviridenko antonsviridenko deleted the dewyatt-839-exceptions-ffi-boundary branch July 22, 2020 14:24
@ni4 ni4 added this to the v0.14.0 milestone Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not allow exceptions beyond the FFI boundary
4 participants