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

Improve error handling #75

Merged
merged 2 commits into from
Dec 3, 2020
Merged

Improve error handling #75

merged 2 commits into from
Dec 3, 2020

Conversation

tstenner
Copy link
Collaborator

Let's take a simple example:

std::vector<std::string> formats = {"", "float32", "float64", "string", "int32", "int16", "int8", "int64"};
std::string format = "double64";
// convert the string to the format index
int fmt = std::distance(formats.begin(), std::find(formats.begin(), formats.end(), format);
try {
    lsl::stream_info info(streamname, "EEG", 1, 500, fmt);
} catch(..) { std::cout << "Error!" << std::endl; return; }
std::cout << info.created_at() << std::endl;

On a first glance, this should print the time the stream_info was created at.

On a second glance, the format is called float64, not double64 so an exception should be thrown and the function should return.

In reality, a lsl::stream_info object is created, but the internal pointer is set to 0 so the call to info.created_at() will dereference a null pointer and everything will crash.

This PR first adds the helper macro LSL_CATCH_EXCEPTIONS and the helper function create_object_noexcept for the C-API to convert exceptions to error codes and copy the error message to an internal variable. The last error message can then be retrieved via the lsl_last_error() function.

After that, the C++ wrapper can check for errors (i.e. is the returned pointer valid) or throw an exception with the last error message.

@tstenner
Copy link
Collaborator Author

Example from the unit tests:

CHECK_THROWS(lsl::stream_info("", "emptyname"));
CHECK(std::string("The name of a stream must be non-empty.") == lsl_last_error());

@chkothe
Copy link
Contributor

chkothe commented Nov 26, 2020

That does look pretty good, and I do like the generic implementation. Also nicely declutters some code.

There's one thing that would make sense to tackle here, and that's that the lsl_last_error wouldn't be thread safe (can return garbled data if some other thread is writing an error message at the same time). I've seen that addressed not with a lock (while it then can't return garbled data anymore, can still return the wrong error message from another thread), but by putting that variable in thread-local storage, so that each thread sees its own version of the last error message. Do you want to give that a shot?

Btw I didn't see the last error being zero-initialized.

@tstenner
Copy link
Collaborator Author

There's one thing that would make sense to tackle here, and that's that the lsl_last_error wouldn't be thread safe (can return garbled data if some other thread is writing an error message at the same time).

On the one hand "My code makes errors faster than I can check them" is a very specific issue, on the other hand it's literally two changed lines (not including the unit test). Added in 95f8277.

Btw I didn't see the last error being zero-initialized.

Done.

Copy link
Contributor

@chkothe chkothe left a comment

Choose a reason for hiding this comment

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

Great stuff, looks clean to me.

@tstenner tstenner merged commit 232b51b into sccn:master Dec 3, 2020
@tstenner tstenner deleted the errorhandling branch December 3, 2020 22:06
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.

None yet

2 participants