Completion of error handling

[elfring]

I have looked at a few source files for your current software. I have noticed that some checks for return codes are missing.

Would you like to add more error handling for return values from functions like the following?

• fclose ⇒ ImageLoader::writeJpg
• pthread_mutex_init ⇒ MutexImpl
• pthread_key_create ⇒ ThreadLocalImpl

[LaurentGomila]

Interesting. What would you do for these specific cases?

[MarioLiebisch]

Not sure about the others, but for the first one using return !fclose(file); should be fine I assume. Or better clear the buffer/contents and then try closing the file again?

[skyrpex]

What about using a std::unique_ptr with custom deleter for the FILE in ImageLoader?

[elfring]

Are you interested to apply aspect-oriented software development?
How do you think about to encapsulate error detection and corresponding exception handling as a reusable aspect in your software?

[LaurentGomila]

for the first one using return !fclose(file); should be fine I assume

I don't think so. First, everything has been loaded successfully so I don't see why we would return a failure. Secondly, it doesn't solve the real problem that your file failed to close properly (and thus is still open).

What about using a std::unique_ptr with custom deleter for the FILE in ImageLoader?

How would that handle the error code returned by fclose?

Are you interested to apply aspect-oriented software development?
How do you think about to encapsulate error detection and corresponding exception handling as a reusable aspect in your software?

I'm not going to change error handling in SFML right now. But I'm open to ideas for SFML 3.

[MarioLiebisch]

I don't think so. First, everything has been loaded successfully so I don't see why we would return a failure.

This is about writing, not loading? (At least in this instance. For code that's loading, I'd say the return value is not as important.)

Secondly, it doesn't solve the real problem that your file failed to close properly (and thus is still open).

That's true, but again not really sure how to solve this properly for all cases.

[LaurentGomila]

This is about writing, not loading? (At least in this instance. For code that's loading, I'd say the return value is not as important.)

Oops 😄

That's true, but again not really sure how to solve this properly for all cases.

That's my point. Sometimes there's no way to properly handle an error case, and the best solution is to do nothing.

The error that is likely to occur with fclose, is a failure to flush the last chunk of buffered data because the disk is running out of space. In this case what should we do? Report failure and try to revert all our changes? If the file was newly created, we must remove it and possibly have to handle other file system errors... this is an endless task. If the file was already existing and was overwritten by the function, reverting is impossible.

So we should definitely do nothing, now the question is: should we report failure or success? I'd say that we should report a failure in this case.

By the way, funny reading: https://groups.google.com/forum/#!msg/comp.lang.c/bc46Pp8wiM4/alPdYP3DAcYJ

[Bromeon]

That's really a problem with no 100% satisfying solution. C++ streams close the file in the destructor and thus have no option of reporting failure. In Java, the developers tried to fix this problem by requiring checked exceptions, which didn't quite go the way they intended -- it became almost a common idiom to write empty try-catch clauses around close() calls.

I agree that it would be good to at least report the error, because a user getting true from saveToFile() expects that the written file is now usable. By the way, another reason why fclose() could fail is the loss of physical access to the file, e.g. in a network drive or external storage device.

One problem of the current API is also that there's no way to know what kind of error happened, so the user is lost if the call fails.

[LaurentGomila]

One problem of the current API is also that there's no way to know what kind of error happened, so the user is lost if the call fails.

Using exceptions in SFML 3 should solve this problem.

[Bromeon]

This specific one yes, but exceptions can't be applied everywhere. For example, classes that keep a file handle open and close it in their destructor (such as sf::Font or sf::Music) can't throw exceptions, because throwing exceptions from destructors is very hazardous.

[retep998]

Throwing an exception from a destructor is only an issue when there is already an unhandled exception so the stack is unwinding, in which case std::terminate is called.

[Bromeon]

Are you suggesting we should throw exceptions nevertheless? Or use std::uncaught_exception() (which is inherently flawed) for conditional throwing? ;)

No, no, never let exceptions leave destructors, it will only wreak havoc. See also GotW 47.

[retep998]

At the very least we should have a .close() method that can throw exceptions.

[LaurentGomila]

Is it realistic to consider a failure when closing a file in read-only access? And even if it happened, the app would still behave as expected (I'm still talking of sf::Font and sf::Music), so why would we throw an exception there?

[germinolegrand]

Le 19/02/2014 15:35, Peter Atashian a écrit :

At the very least we should have a |.close()| method that can throw
exceptions.

—
Reply to this email directly or view it on GitHub
#533 (comment).

+1 that's the design of std::basic_*stream, which sets a failbit on
failure. By exception, error code, state bits, choose one, they can do
what you want.

[TankOs]

What about postponing this to SFML 3?

[LaurentGomila]

If something can be done right now we should do it, so that when we switch to exceptions in SFML 3, the code to modify will already be clearly identified. But if the solution is not so obvious, yes, let's forget about it for now.

[Bromeon]

Some of them can be solved in SFML 2.x without exceptions. For example, if fclose() fails, we can return false in saveToFile(). Keep in mind that the file may be buffered and fclose() might flush this buffer -- so if the data cannot be written, there will be an error.

Mutex and key (for thread-local storage) construction fails if there are not enough system resources, and destruction fails if a lock is still held. The standard std::mutex class yields UB in the latter case.

As mentioned earlier, it's a bad idea to throw exceptions from destructors. But if SFML 3 uses C++11, the classes sf::Mutex and sf::ThreadLocal<T> will become obsolete because of std::mutex and thread_local.

[binary1248]

So... is this issue still scheduled for fixing before SFML 3? If a more or less complete list can be provided of sites where error codes aren't checked, I can probably come up with a fix for this.

[eXpl0it3r]

I'd say we should fix it before SFML 3. The question is, If there are more of these code pieces that don't generate an error up on failing.

[binary1248]

So... to create a sort of agenda for this issue...

What needs to be done:

• Find all the places where errors can happen, but are not currently handled
• Handle them in a suitable way (taking into consideration we are still C++03 and SFML doesn't make use of exceptions yet)
• When they can't be handled in a suitable way (e.g. in some constructors/destructors), at least print something out on the console?