sccn / liblsl

C++ lsl library for multi-modal time-synched data transmission over the local network
Other
108 stars 63 forks source link

Improve error handling #75

Closed tstenner closed 3 years ago

tstenner commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.