Simplify 'sf::Err' and prevent data races

[vittorioromeo]

Should fix the crash in #3003 and also simplifies sf::Err's implementation by removing DefaultErrStreamBuf. AFAICS, that custom stream buffer was used to make error printing buffered, as std::cerr is unbuffered by default.

But I don't think the extra complexity is worth it, we're basically optimizing the performance of... error message printing.

Am I missing something?

[eXpl0it3r]

It's been implemented like that since the pre git days, unfortunately I don't know how to find FS#10 😄
But it might be safe to assume, that this might not be needed anymore.

76de05a#diff-bd73b69ba08ad3c155e32487a2d553ed69f7ec4cbea6fa3ef7a5fcb5a95ecab8

[ChrisThrasher]

we're basically optimizing the performance of... error message printing.

I'm not stressed about the performance of error paths so if we can fix threading issues and removes lots of code at the cost of error-path performance then I'm cool with that. Seems like an easy win.

Let's see if those experiencing crashes can still recreate it with this patch.

[ChrisThrasher]

If we simplify sf::err down this far, then if I'm understanding correctly the only reason this API exists is to provide a way to redirect the SFML error messages without redirecting all of std::cerr. Is that right? I suppose in theory someone may want to send all SFML errors to a different stream or silence them entirely but not affect std::cerr which they may still be using for other purposes.

[vittorioromeo]

If we simplify sf::err down this far, then if I'm understanding correctly the only reason this API exists is to provide a way to redirect the SFML error messages without redirecting all of std::cerr. Is that right? I suppose in theory someone may want to send all SFML errors to a different stream or silence them entirely but not affect std::cerr which they may still be using for other purposes.

Yes, I believe this is done in MainAndroid.cpp:

// Redirect error messages to logcat
sf::err().rdbuf(&states->logcat);

[ChrisThrasher]

If sf::err is directed at std::cerr then it inherits the thread safety guarantees of std::cerr. However if we redirect sf::err elsewhere, those thread safety guarantees don't necessary continue, right? In that case can we still call sf::err thread safe?

Somewhat tangential but I wonder if the sf::err tests ought to do some multithreading to better test this use case. It's the one place in SFML where I think multithreading tests may be worthwhile.

[vittorioromeo]

If sf::err is directed at std::cerr then it inherits the thread safety guarantees of std::cerr. However if we redirect sf::err elsewhere, those thread safety guarantees don't necessary continue, right? In that case can we still call sf::err thread safe?

Somewhat tangential but I wonder if the sf::err tests ought to do some multithreading to better test this use case. It's the one place in SFML where I think multithreading tests may be worthwhile.

The additional safety that this PR brings is that every thread has its own clone of sf::err. The thread-safety of the buffer sf::err writes to is unchanged from this PR.

Prior to this PR, if the buffer used was thread-safe, we would still get data races because sf::err was a global variable shared between threads without any synchronization.

After this PR, if the buffer used is thread-safe, sf::err will be safely usable concurrently.

[ChrisThrasher]

The additional safety that this PR brings is that every thread has its own clone of sf::err.

Right, duh, thread_local to the rescue.

For users who want to silence or redirect SFML log messages, they now have to do that in each thread individually. Not sure how much crossover there is between those who redirect sf::err and those who use sf::err concurrently but this may be something we want to explain in the API docs since it's not necessarily intuitive. Prior to this you only needed one call to sf::err().rdbuf to redirect all logs in all threads.

[eXpl0it3r]

For users who want to silence or redirect SFML log messages, they now have to do that in each thread individually.

Which you can't really, since for example the audio threads are spawned by SFML and not accessible for redirection?

[vittorioromeo]

The additional safety that this PR brings is that every thread has its own clone of sf::err.

Right, duh, thread_local to the rescue.

For users who want to silence or redirect SFML log messages, they now have to do that in each thread individually. Not sure how much crossover there is between those who redirect sf::err and those who use sf::err concurrently but this may be something we want to explain in the API docs since it's not necessarily intuitive. Prior to this you only needed one call to sf::err().rdbuf to redirect all logs in all threads.

That's a very good point! I'll add something in the documentation for sure. The alternative is to have a single global buffer pointer that all thread-local instances share, but then we introduce back complexity that I don't think is needed.

[ChrisThrasher]

Which you can't really, since for example the audio threads are spawned by SFML and not accessible for redirection?

Interesting point. Not all threads are owned and accessible by users.

Vittorio floated the idea of a std::print-like interface. If we switched to this then we could have one function for printing messages and a separate function for setting the destination. This 2nd function could be implemented such that all thread-local instances of the underlying ostream are ensured to use that destination. Just thinking out load here so pardon me if this idea has some issues.

[vittorioromeo]

For users who want to silence or redirect SFML log messages, they now have to do that in each thread individually.

Which you can't really, since for example the audio threads are spawned by SFML and not accessible for redirection?

True.

So we'd have to do something like

std::atomic<std::streambuf*> errBufPtr{std::cerr.rdbuf()}; // global, private

void redirectErrStream(std::streambuf* bufPtr); // public

std::ostream& err()
{
    thread_local std::ostream stream;
    stream.rdbuf(errBufPtr.fetch());

    return stream;
}

[ChrisThrasher]

I wrote some multithreading tests that are running against this PR. These tests often pass but sometimes fail due to certain strings not being present in the final buffer. I suspect this is because we have 5 threads writing to the same std::stringstream without synchronization. Is this a valid use case? It's certainly something the API currently allows. Having separate std::ostream per thread doesn't protect users against those streams pointing to the same buffer.

[System] sf::err
  Print from multiple threads
-------------------------------------------------------------------------------
/Users/thrasher/Projects/sfml/test/System/Err.test.cpp:46
...............................................................................

/Users/thrasher/Projects/sfml/test/System/Err.test.cpp:63: FAILED:
  CHECK_THAT( stream.str(), Catch::Matchers::ContainsSubstring(std::to_string(i)) )
with expansion:
  "1234" contains: "0"

Is this something we just need document in hopes that users avoid it? Not sure what we can do from an API design perspective to prevent this so long as sf::err() is still a std::ostream& that anyone can manipulate from any thread.

[vittorioromeo]

I think I can come up with an alternative design that uses a single err stream that locks at the beginning of a streaming chain and unlocks automatically at the end. That would avoid the issues with the buffer redirection. Will do it tomorrow

[vittorioromeo]

Closing this in favour of #3010 which I think is superior because it has fewer caveats.