UCI2WB 4.0

[hgm]

however the current UCI2WB code is not good because it doesn't check the result of fprintf

Just out of curiosity: How could fprintf ever return a failure?

[Ras]

What "is waiting" means?

Being idle, waiting for commands. I.e. not searching.

Do you know any engine which will not handle the sequence of commands I posted?

Irrelevant. The adaptor should work even with engines that don't exist yet so that their implementation is not known now. Btw., there is one corncer case where even buffering will cause trouble, and that's if setoption is used for variants like Chess960 where the chess rules are modified. If GUI and engine don't agree on the applicable chess rules that the "bestmove" should adhere to, that will cause issues.

Just out of curiosity: How could fprintf ever return a failure?

In case the stream is a file, then anything could go wrong. Disk full, disk removed, network connection down with remote files. With stdout as stream, that would only be if the pipe breaks, i.e. if the receiving program has exited. But that is already checked here I think:

Code: Select all

if(!fromF && !ReadLine(fromE, line)) printf("tellusererror UCI2WB: %s died on me\n", binary), exit(0);

[hgm]

Btw., there is one corncer case where even buffering will cause trouble, and that's if setoption is used for variants like Chess960 where the chess rules are modified. If GUI and engine don't agree on the applicable chess rules that the "bestmove" should adhere to, that will cause issues.

I don't get this. Surely it should not be possible to change variant while the engine is thinking. The variant should already be known whenever a search is started. Or even when the position is set up, as otherwise you would not know what 'startpos' means, or how to parse the FEN.

In case the stream is a file, then anything could go wrong. Disk full, disk removed, network connection down with remote files. With stdout as stream, that would only be if the pipe breaks, i.e. if the receiving program has exited. But that is already checked here I think:

Code: Select all

if(!fromF && !ReadLine(fromE, line)) printf("tellusererror UCI2WB: %s died on me\n", binary), exit(0);

That is what I thought. Of course it would be theoretically possible that an engine just closes stdin, but leaves stdout open, so that reading from the engine would not detect it. But that is in principle not different from the engine simply refusing to read from the pipe, (or ignore what it reads), which could not be checked from fprintf. So it seems pointless to test for this special case (which looks more like a poorly executed attempt at sabotage anyway...).

[Ras]

I don't get this. Surely it should not be possible to change variant while the engine is thinking. The variant should already be known whenever a search is started.

Sure, but if the variant is changed via setoption while the engine is searching, then there are two possibilities if the engine doesn't outright discard setoption:

1) The engine buffers setoption while searching, does the search as per the old variant, and the resulting bestmove adheres to the old variant. Only afterwards, the engine changes the variant.
2) The engines processes setoption even during search, changes the variant, and the resulting bestmove adheres to the rules of the new variant.

That's why it's good that the adaptor buffers setoption when the engine is not waiting for commands.

But that is in principle not different from the engine simply refusing to read from the pipe, (or ignore what it reads), which could not be checked from fprintf. So it seems pointless to test for this special case (which looks more like a poorly executed attempt at sabotage anyway...).

I agree on both points. It is generally suggested that it is a "very bad idea"(tm) to fclose stdin except right before terminating the application because it would mess with the file descriptors. Even for remapping stdin/out/err, freopen instead of an fclose/fopen is suggested.

Maybe a regular isready would be appropriate because that would also detect whether the engine just hangs somehow, which would be a more plausible error scenario. The good thing is that isready has to be answered also during search so that a hanging engine is easy to detect.

[Fulvio]

Code: Select all

if(!fromF && !ReadLine(fromE, line)) printf("tellusererror UCI2WB: %s died on me\n", binary), exit(0);

You should check all the I/O!!
What is that "ReadLine" without the size of the buffer?
If the engine sends a line longer than 1024 bytes you have a buffer overflow!
Please use a static analyzer like scan.coverity.com (free for open source projects): I'm sure there are many more security issue in that code!
Or even better: use a library instead of re-inventing the wheel!

[Fulvio]

Do you know any engine which will not handle the sequence of commands I posted?

Irrelevant.

Let's stop here.
I stand by my opinion: if an engine receives the sequence
"stop"
"setoption ..."
and decides to process "setoption" before interrupting the search, it's a bug in the engine.

[Ras]

I stand by my opinion: if an engine receives the sequence
"stop"
"setoption ..."
and decides to process "setoption" before interrupting the search, it's a bug in the engine.

I agree to that, but that was not the question. Please re-read what the discussion with buffering setoption actually was about.

[brianr]

OK, I uploaded a patched version. Compared to 4.0 this has 2 extra features for prevending hanging engine processes:

1) An extra 'quit' command will be sent to the engine directly from the GUI thread (bypassing the command queue, which might be blocked while waiting for the engine) about 500msec after a quit command is received. This solves the problem for engines that take a long time to initialize (so that the engine thread is tied up waiting for 'uciok'), when we want to quit them during initialization.
2) A second 'new' command to the same UCI2WB run will now skip the 'isready' handshake. Since UCI2WB specifies feature reuse=0 such a second 'new' should never be needed, but WinBoard spuriously sends one just before it quits the engine to launch a new one. With non-compliant engines (such as Stockfish 10, and many USI engines) that fail to respond to the 'isready' this would lead to hanging engine processes when WinBoard finally killed the adapter that was waiting for the 'readyok' that would never come.

I also fixed a synchronization problem that would sometimes lead to starting of a spurious search during loading of a game (where moves could arrive changing the side to move while a preceding 'force' command still resided unexecuted in the command queue).

The downloads with the new UCI2WB are at:

Bare UCI2WB.exe: http://hgm.nubati.net/UCI2WB.zip
WinBoard-AA bundle: http://hgm.nubati.net/WinBoard-AA.zip

Finally got around to testing this patched v4 version again with Leela (lc0.exe).
Extra lc0.exe processes are still created (one for each game, I think at the end of a game but not sure).
The extra .exes are all very small (11MB, the "full" lc0.exe is about 850MB), and they don't use any CPU time.
This does not happen with Polyglot.

Thanks again for all of your work with Winboard.

[hgm]

I finally got to fixing the problem the new UCI2WB had with playing games from an opening line. (By having the engine thread also handle appending of moves to the game, so that it now pretty much handles everything. The the GUI thread is only responsible for immediate execution of force, quit and move-now commands, or processing of ponder hits/misses, and queues all other commands for execution by the engine thread when it is no longer waiting for engine output.)

It also fixes a problem with the translation of the UCI_Variant option (which did append garbage characters to the name of the last variant that option could be set to). I gave it version number 4.1; it can be downloaded both as part of the WinBoard-AA package as a bare executable, at the same links as above. (Beware of browser caching!)

[hgm]

I now also updated the WinBoard executable in the AA package. This fully implements the new method for position setup in the mode /pieceMenu=true (which no longer involves a piece menu!). To get it in that mode you might have to run it once with this as 'Additional option' (because your saved settings might have /pieceMenu=false).

Another fix is that walking the PV of a pondering engine now pays attention to a ponder move given by the engine as Hint command, so that walking the PV is possible even when the engine does not prefix it with the ponder move.