search

dougbinks / enkiTS

A permissively licensed C and C++ Task Scheduler for creating parallel programs. Requires C++11 support.
zlib License
1.74k stars 145 forks source link

Feature request: check return codes for semaphore system calls #35

Closed boxerab closed 5 years ago

boxerab commented 5 years ago

I noticed that the return codes for semaphore creation etc. aren't being checked for errors. So, create may fail and caller doesn't get notified. What do you think is the best way of handling error conditions: throw an exception, or return false ? Thanks!

dougbinks commented 5 years ago

I think that error handling for well formed calls to semaphore creation etc. would be superfluous, as in the situation that these occur the system is likely in a state where there isn't much the app can do but crash, which will happen when the semaphore is used. There is some potential utility in logging that this is a system issue rather than a app one, but this will be caught by a callstack handler like breakpad etc. when the semaphore use call fails and is likely so rare as to be overkill in adding specific handling.

I'm willing to be convinced though.

I should probably add a few more asserts though to ensure that the code is well formed, and that programming errors are easily caught.

Exceptions aren't something I'd use, so if we did add error handling it would be to return a error state and either add a GetLastError or error callback.

boxerab commented 5 years ago

Yes, it's fine then as it is. I'm just in the habit of always checking return codes, no matter what :)

boxerab commented 5 years ago

@dougbinks I am re-opening this as I found a semaphore implementation, cpp11-on-multicore, that does check return codes for certain platforms.

https://github.com/preshing/cpp11-on-multicore/blob/master/common/sema.h

Interested to know your thoughts on this.

dougbinks commented 5 years ago

Preshing has an excellent set of libraries for atomics and multithreading, and his blog posts on C++11 atomics are a must-read.

The only error check I can see is in the Posix implementation of wait(), where gdb fails to wait the first few calls when attached to the debugger (actually it's when interrupted by a signal handler). That's worth adding to the code.

boxerab commented 5 years ago

Thanks, yes, this would be good to have in there.

dougbinks commented 5 years ago

I've added a check with this commit: https://github.com/dougbinks/enkiTS/commit/503f006e4b685f69bd49f361a2aac4e9d487286d

Let me know if this resolves this issue for you.

boxerab commented 5 years ago

Great, thanks! This commit also removes a compiler warning about unused variable err.

boxerab commented 5 years ago

Closing.