Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.90%. Comparing base (15c547e) to head (3e4700c).
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main    #1695      +/-   ##
==========================================
- Coverage   53.90%   53.90%   -0.01%     
==========================================
  Files         341      341              
  Lines       27983    27985       +2     
==========================================
+ Hits        15084    15085       +1     
- Misses      12899    12900       +1     

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Maybe I am missing something, but not closing the pipe can potentially make the problem much worse. If the solution sends multiple queries and then it exits, the controller will try to print answers for all the queries, filling up the pipe buffer and blocking indefinitely.

I would argue that it is much better to ignore both SIGPIPE and write errors inside the controller.

That issue can occur regardless. A solution which sends a large number of queries without reading responses will cause the checker->solution pipe to fill up, which causes the checker to stop processing queries, and then after a while the solution->checker pipe fills up too and we get a deadlock. In Kattis, this has been mitigated somewhat by increasing pipe buffer size to the Linux maximum of 1 MiB, and it is eventually planned to be fixed for real by introducing a proxy process (see Kattis/problemtools#113; additional upsides of that solution is being able to log the interaction and show it for sample testcases, and being able to impose communication traffic limits to eliminate judge errors from resource exhaustion caused by unbounded-size cin >> str; in checkers; downsides are complexity and increased judging walltime).

I suppose a small difference compared to that scenario is that with the pipe left open the checker could be the one left to take the blame, if the solution exits early and the judging system treats checker walltime limit timeouts as judge errors.

Anyway, this not being a hill I want to die on, I just pushed a commit that closes the pipe.

i can think of a way to avoid the deadlock, though it requires a bit of extra complexity on the manager side.

• set the manager->solution pipe as O_NONBLOCK.
• when writing to the solution, attempt a write(); if it fails with EAGAIN, stick the data into a queue.
• when reading, if there's any queued data then instead of reading directly, do a poll() on both pipes; if the writing pipe has space now, write some data from the queue into it before proceeding with the read.

while this is somewhat complicated, i think it can be abstracted away into a header-only library a la testlib.h.

i think this should have minimal performance impact if the solution is well-behaved and reads its end of the pipe normally (in which case the write() should rarely fail at all, at least when assuming that the pipe buffer is large enough for one query's response). in any case, it avoids the overhead of another proxy process and all the associated context switches.

Sure, that also works. I'm not the biggest fan of putting logic like this into the manager itself, admittedly, it means that old problems need be updated in order to behave properly, and it's hard to ensure task authors use the latest testlib instead of copy-pasting from an old problem.

I'm fine with the current version of the PR.

Regarding deadlocks: I do not think that reporting deadlocks in communication is worth the complexity (and time overhead) it requires. Just let the evaluation hit the wall time limit in the rare cases when this happens.