Give each engine its own I/O instead of hijacking stdin and stdout - #13
Open
veloce wants to merge 6 commits into
Open
Give each engine its own I/O instead of hijacking stdin and stdout#13veloce wants to merge 6 commits into
veloce wants to merge 6 commits into
Conversation
Every flavour is a separate library with its own engine state, but each of
them talked to the outside world through the process's own descriptors: the
shim dup2'd its pipe onto fd 0 and fd 1, and the engine read std::cin and
wrote std::cout. Two flavours therefore could not be resident at the same
time -- the second redirection won and both engines' output arrived in one
channel -- and the host application lost its own stdout while an engine ran.
Each package now carries an fd-backed streambuf pair in src/sfio.{h,cpp},
declared in that engine's own namespace, and the shim points it at its pipe
with sfio::bind() in place of the dup2.
Most of the engine side is covered by redefining sync_cout, which accounts
for nearly all output including every info and bestmove line. The rest are
the getline() calls, the raw std::cout in the "bestmove ... ponder" tails,
tune.cpp, Fairy's parser.h, and -- less obviously -- the Logger in misc.cpp,
which swaps rdbuf on the streams and would otherwise have silently stopped
capturing anything, plus SF 18's sync_cout_start/end and print_info_string,
a second output path that does not go through the macro.
The streams are tied the way std::cin is tied to std::cout, which upstream
depends on: lines written with '\n' rather than a flush -- "info string"
among them -- only reach the GUI because the next read pushes them out.
SF_PHASE_REDIRECTING and SF_MAIN_DUP2_FAILED become SF_PHASE_BINDING_STREAMS
and SF_MAIN_BIND_FAILED, keeping their numbers, and StockfishPhase.redirecting
becomes bindingStreams. Neither has shipped yet.
The shim test now also checks that the process keeps its own stdin and stdout.
A new host test links sf16 and Fairy-Stockfish into one binary, the way iOS
links them, and searches on both at once to confirm that neither one's traffic
reaches the other's channel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces private, per-engine I/O streams so multiple Stockfish flavours can run concurrently without replacing host process descriptors.
Changes:
- Adds fd-backed stream buffers across all three native engines.
- Updates engine output, logging, diagnostics, and lifecycle constants.
- Adds integration coverage for descriptor preservation and concurrent flavours.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
test/two_flavours_test.cpp |
Tests concurrent sf16 and variant engines. |
test/run_two_flavours_test.sh |
Builds and runs the concurrent-engine test. |
pkgs/multistockfish/lib/src/stockfish_diagnostics.dart |
Renames stream-binding diagnostics. |
pkgs/multistockfish/CHANGELOG.md |
Documents private engine I/O. |
pkgs/multistockfish_variant/test/shim_test.cpp |
Tests host descriptor preservation. |
pkgs/multistockfish_variant/test/run_shim_test.sh |
Updates shim test execution. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/stockfish_variant.cpp |
Binds Fairy-Stockfish to private streams. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/include/multistockfish_variant/stockfish_variant.h |
Updates native phase and error constants. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/uci.cpp |
Routes UCI input privately. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/tune.cpp |
Routes tuning output privately. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/sfio.h |
Declares Fairy-Stockfish private streams. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/sfio.cpp |
Implements fd-backed Fairy-Stockfish streams. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/search.cpp |
Routes best-move output privately. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/parser.h |
Routes parser diagnostics privately. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/misc.h |
Redirects synchronized output macro. |
pkgs/multistockfish_variant/ios/multistockfish_variant/Sources/multistockfish_variant/Fairy-Stockfish-2b5d9512/src/misc.cpp |
Adapts logging to private streams. |
pkgs/multistockfish_variant/CHANGELOG.md |
Documents variant I/O changes. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/uci.cpp |
Routes sf16 UCI input privately. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/tune.cpp |
Routes sf16 tuning output privately. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/sfio.h |
Declares sf16 private streams. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/sfio.cpp |
Implements fd-backed sf16 streams. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/search.cpp |
Routes sf16 best-move output privately. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/misc.h |
Redirects sf16 synchronized output. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/Stockfish16/src/misc.cpp |
Adapts sf16 logging. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/stockfish16.cpp |
Binds sf16 to private streams. |
pkgs/multistockfish_sf16/ios/multistockfish_sf16/Sources/multistockfish_sf16/include/multistockfish_sf16/stockfish16.h |
Updates sf16 lifecycle constants. |
pkgs/multistockfish_sf16/CHANGELOG.md |
Documents sf16 I/O changes. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/uci.cpp |
Routes Stockfish 18 UCI I/O privately. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/tune.cpp |
Routes tuning output privately. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/sfio.h |
Declares Stockfish 18 private streams. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/sfio.cpp |
Implements fd-backed Stockfish 18 streams. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/misc.h |
Redirects synchronized output. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/Stockfish/src/misc.cpp |
Adapts logging and output locks. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/stockfish_nnue.cpp |
Binds Stockfish 18 to private streams. |
pkgs/multistockfish_chess/ios/multistockfish_chess/Sources/multistockfish_chess/include/multistockfish_chess/stockfish_nnue.h |
Updates Stockfish 18 lifecycle constants. |
pkgs/multistockfish_chess/CHANGELOG.md |
Documents Stockfish 18 I/O changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+16
to
+17
| Until bind() is called they read and write the process's standard descriptors, | ||
| which keeps a plain command-line build of the engine working unchanged. |
Comment on lines
+16
to
+17
| Until bind() is called they read and write the process's standard descriptors, | ||
| which keeps a plain command-line build of the engine working unchanged. |
Comment on lines
+16
to
+17
| Until bind() is called they read and write the process's standard descriptors, | ||
| which keeps a plain command-line build of the engine working unchanged. |
| # stdout is left alone on purpose: one of the things this checks is that the | ||
| # engines no longer take the process's stdout over, so anything appearing on | ||
| # stdout here would itself be a failure. | ||
| "$out/two_flavours_test" 2>&1 >/dev/null |
| # The test reports on stderr and leaves stdout alone on purpose: one of the | ||
| # things it checks is that the engine no longer takes the process's stdout over, | ||
| # so anything appearing on stdout here would itself be a failure. | ||
| "$out/shim_test" 2>&1 >/dev/null |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/run_two_flavours_test.sh:60
- This redirection contradicts the assertion above: any engine output that still leaks through
std::coutis discarded, so it cannot make the test fail. Thefstatcheck only detects descriptor replacement and will still pass for a missed output call site. Capture stdout and require it to remain empty.
pkgs/multistockfish_variant/test/run_shim_test.sh:38 - This discards the exact stdout interference the test claims to detect. The descriptor-identity assertion only catches
dup2; a remaining engine write tostd::coutleaves the descriptor identity unchanged and passes unnoticed. Capture stdout and fail when it is non-empty.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Every flavour is a separate library with its own engine state, but each of them talked to the outside world through the process's own descriptors: the shim dup2'd its pipe onto fd 0 and fd 1, and the engine read std::cin and wrote std::cout. Two flavours therefore could not be resident at the same time -- the second redirection won and both engines' output arrived in one channel -- and the host application lost its own stdout while an engine ran.
Each package now carries an fd-backed streambuf pair in src/sfio.{h,cpp}, declared in that engine's own namespace, and the shim points it at its pipe with sfio::bind() in place of the dup2.
The streams are tied the way std::cin is tied to std::cout, which upstream depends on: lines written with '\n' rather than a flush -- "info string" among them -- only reach the GUI because the next read pushes them out.
Assisted with Claude Opus 5.