Use anonymous pipes for stdout/stderr redirection.

[davidmatson]

Closes #2444.

Avoid the overhead of creating and deleting temporary files.
Anonymous pipes have a limited buffer and require an active reader to
ensure the writer does not become blocked. Use a separate thread to
ensure the buffer does not get stuck full.

A couple of questions:

1. Is adding C++20-conditional usage of jthread the recommended approach here? (The code as written prefers std::jthread when available but falls back to a custom implementation when needed.)
2. Is adding CATCH_SYSTEM_ERROR reasonable here? (system_error seems to be the best exception type for these APIs)

[codecov]

Codecov Report

Merging #2472 (3b474ce) into devel (5a1ef7e) will decrease coverage by 0.20%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##            devel    #2472      +/-   ##
==========================================
- Coverage   91.49%   91.28%   -0.20%     
==========================================
  Files         181      185       +4     
  Lines        7519     7596      +77     
==========================================
+ Hits         6879     6934      +55     
- Misses        640      662      +22     

[davidmatson]

@horenmar - Please let me know what you think of how this approached worked out.

[davidmatson]

@horenmar - it looks like you triggered a new validation run (thanks!). For this one:
Linux builds (complex) / CMake configuration tests, clang++-10, C++14 Debug (pull_request)
I'm guessing that's a new failure in this PR, but I'm not sure what the root cause is. The error message says:

Linking CXX executable SelfTest
/usr/bin/ld: ../src/libCatch2d.a(catch_output_redirect.cpp.o): in function `std::thread::thread<Catch::OutputFileRedirector::OutputFileRedirector(_IO_FILE*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)::{lambda()#1}, , void>(Catch::OutputFileRedirector::OutputFileRedirector(_IO_FILE*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&)::{lambda()#1}&&)':
/usr/include/c++/9/thread:126: undefined reference to `pthread_create'
collect2: error: ld returned 1 exit status
make[2]: *** [tests/CMakeFiles/SelfTest.dir/build.make:867: tests/SelfTest] Error 1
make[1]: *** [CMakeFiles/Makefile2:927: tests/CMakeFiles/SelfTest.dir/all] Error 2
make: *** [Makefile:166: all] Error 2

For pthread_create - that's not something I wrote directly (actually I can't find "pthread_create" at all in this repo). I'm guessing it's some kind of build setting needed on Linux since it's using threads. The closest reference I could find in the repo was:

#if defined(_REENTRANT) || defined(_MSC_VER)
// Enable async processing, as -pthread is specified or no additional linking is required
# define CATCH_INTERNAL_CONFIG_USE_ASYNC

Is there some place -pthread needs to be enabled in a build script somewhere for this change to work? (Enabling threading on Linux is something I'm not familiar with.) Thanks!

[horenmar]

it looks like you triggered a new validation run (thanks!)

Technically, first one. 😃 GHA don't run by default for PRs by new contributors, because people are asshats and try to use free resources for mining. But that's not the important part.

---

The underlying issue is that libstdc++ (and IIRC libc++ as well) rely on pthread library for their implementation of std threading support. This means that building code that uses std::thread & friends requires passing -pthread/-lpthread* to the build.

The code you've found detects whether Catch2 is being compiled in a way that allows us to optionally use threads (MSVC always has thread support, _REENTRANT is defined by GCC/Clang when compiling a TU with -pthread).

Enabling thread support for a target in CMake is simple enough, but the issue is whether doing so is something I want to do.

1. Catch2 tries to support platforms with limited stdlib support (e.g. iostreams are optional, RTTI is optional, exceptions are optional), and I don't think that this is enough motivation to make threads an unconditional requirement.
2. The amalgamated distribution can't actually affect its compilation options in any way.

This would mean that the threading requirement would have to be optional and the implementation of the experimental capture would have to properly handle the option that there are no threads enabled.

One more thing I don't like about using threads in the experimental capture is that given the issue in 1), I don't think it could ever be promoted into non-experimental implementation.

---

* Actually it is slightly more complex, as the compiler flag does something different than the linker flag, and their behaviour still differs between GCC and Clang. Let nobody say that the C++ toolchains are sane.

[davidmatson]

@horenmar - thanks for the reply!

With pipes, something needs to keep reading the pipe, or the OS buffer can get full and the writer will block.

The only solutions I'm aware of for this problem are:
a. Use a dedicated thread for each pipe, to ensure it keeps getting drained.
b. Use an asynchronous read/poll on each pipe, and switch to reading from it when it is signaled.
c. Ensure that the total amount of data ever written to a pipe is smaller than the OS buffer.

We could investigate option b, though I suspect platform support for non-blocking reads is less common than threads.

I can understand the desire to support Catch2 on more-limited platforms and thus avoid the use of threads as a hard requirement. At the same time, I think for capturing stdout/stderr, pipes are the best solution, as they avoid the problems with temp files we've seen earlier. Other currently problems with files seem likely either just to go away or be more readily soleable as well (perhaps including #1902, #1991).

A few overall options I can think of:

1. Only support redirecting stdout/stderr on platforms that have support for threads.
2. Add a polling (async read when signaled) implementation, and only support redirecting stdout/stderr on platforms that have support for threads or polling.
3. On platforms where threads are not available, rely on the OS buffer and document that stdout/stderr redirection requires threads for larger output volume.
4. Detect which implementation to use at compile-time, and fallback to a file-based solution when threads are unavailable.

There may be other options I'm missing as well.

I tend to think option 4 is likely the best option - thoughts?

EDIT:
Perhaps depending on threads isn't really all that different in terms of whether the experimental implementation can become non-experimental. From this earlier thread:
#2444 (comment)

It [the current experimental implementation] has some compilation issues on less common platforms (e.g. missing posix parts), and IIRC runs into issues when too many tests are run in single binary (IIRC that's due to trying to open too many temp files).

Maybe any implementation that goes all the way down to the handle level is going not to be supported on the most limited platforms, so maybe saying such features only work on more full-featured platforms is just fine, and in keeping with what's possible without this PR anyway. (i.e., so maybe option 1 is more attractive) Thoughts?

[davidmatson]

@horenmar - one option is just to go with the original plan and make the pipes implementation be just for Windows, and keep the existing temp file-based code for other environments. (Though since basically the same code works on Linux as well, when threads are enabled, it would probably make the most sense to have the #if be something like "MSVC/Windows or Pthreads enabled", since that works too.)

I'm not sure what types of more constrained systems you're most interested in supporting, so I'm curious to hear your thoughts on how to consider more limited systems. On that topic, it's worth mentioning that there may be some systems where pipes + threads would be a better option than temp files. For example, Stroustrup mentions in chapter 15 of The C++ Programming Language how some systems compile without using files. At runtime, some systems have no filesystem (or may have a read-only filesystem). Interestingly, I found one person looking for a unit testing framework for a system with no filesystem, and the one he found didn't work because it used a temp file to redirect stdout/stderr:
nemequ/munit#69

So I think which technique works best depends on the system, and I'm not sure which of the following systems are in-scope for what you'd want to support:

System	Best Option
Windows	Pipes + Threads
Other full-featured system, with PThreads	Pipes + Threads
Other full-featured system, without PThreads	Temp files
Limited system with pipes and threads	Pipes + Threads
Limited system without threads or pipes, but with a filesystem (that's writable)	Temp files
Limited system without threads, pipes, or a writeable filesystem	Redirect std::cout/cerr/clog only (can't redirect stdout/stderr)

[horenmar]

I think this is fine, but it will have to be a parallel implementation to the current temp-file based approach, so that there will be 3 different capture impls in the end.

(Also please set up your IDE to use .clang-format from the repo)

[horenmar]

Is adding C++20-conditional usage of jthread the recommended approach here? (The code as written prefers std::jthread when available but falls back to a custom implementation when needed.)

I am not convinced that a quarter-baked jthread polyfill is a good idea. Given that the only feature used from it is defaulting to join, I would rather see the OutputFileRedirector join in destructor by itself.

Is adding CATCH_SYSTEM_ERROR reasonable here? (system_error seems to be the best exception type for these APIs)

🤷 I don't think it will provide useful error messages like this, but that can be fixed later on.

[davidmatson]

I am not convinced that a quarter-baked jthread polyfill is a good idea. Given that the only feature used from it is defaulting to join, I would rather see the OutputFileRedirector join in destructor by itself.

Sure, I've moved it just to join in the destructor.

[davidmatson]

I'm not sure how much coverage there is for both variations of CATCH_CONFIG_NEW_CAPTURE (temp file vs pipe/thread). Ideally, we'd have:

1. Temp file on Linux (CATCH_CONFIG_NEW_CAPTURE but not threads)
2. Pipe/thread on Windows (CATCH_CONFIG_NEW_CAPTURE and CATCH_INTERNAL_CONFIG_USE_ASYNC).
3. Pipe/thread on Linux (CATCH_CONFIG_NEW_CAPTURE and CATCH_INTERNAL_CONFIG_USE_ASYNC).
   and CI would run the testConfigureExperimentalRedirect for each of these cases. I'm not sure what happens currently and which portions of the pipe vs temp file are validated in terms of either compiling correctly or working functionally by CI. (I don't have a way set up to do that locally.)

[davidmatson]

@horenmar - any further thoughts on this PR? thanks!

[davidmatson]

@horenmar - does this PR look good to you? Thanks!