Modernize agent I/O with CompletableFuture and BufferedReader.#79
Modernize agent I/O with CompletableFuture and BufferedReader.#79DomiKoPL wants to merge 1 commit into
Conversation
- Replace busy-wait polling loop in RefereeAgent.getOutput() with CompletableFuture + orTimeout for precise, non-blocking timeouts - Use a dedicated single-thread ExecutorService (AGENT_IO_EXECUTOR) for blocking I/O to avoid ForkJoinPool starvation - Switch to BufferedReader (readLine) instead of raw InputStream reads - Remove unused processStdout InputStream field and abstract getOutputStream() method — only the BufferedReader is needed now - Deduplicate timeout logic: RefereeAgent now delegates to Agent.getOutput() instead of reimplementing it - Clean up unused imports (InputStreamReader, TimeoutException)
sdhoine
left a comment
sdhoine
left a comment
There was a problem hiding this comment.
Detailed review of the diff. I’m requesting changes because the new timeout model introduces regressions that can still create false timeouts and weakens existing output limits.
| try { | ||
| return future.orTimeout(timeout, TimeUnit.MILLISECONDS).join(); | ||
| } catch (CompletionException e) { | ||
| future.cancel(true); |
There was a problem hiding this comment.
CompletableFuture.cancel(true) does not stop the supplier that is already blocked in BufferedReader.readLine(). Since all reads share the single agent-reader executor, one timed-out player that never emits a newline can permanently occupy that thread and make later referee/player reads timeout even when their output is ready. This needs a design where timed-out blocking reads cannot block subsequent reads.
| try { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (int i = 0; i < nbLine; i++) { | ||
| String line = processStdoutReader.readLine(); |
There was a problem hiding this comment.
This removes the previous byte-level cap. readLine() can allocate an arbitrarily large line before the maxOutputSize check below runs, and the later check counts UTF-16 chars rather than UTF-8 bytes. A bot can now emit a huge unterminated line and bypass the intended output limits or cause memory pressure.
| } else { | ||
| if ((offset < AGENT_MAX_BUFFER_SIZE) && (nbOccurences < nbLine)) { | ||
| Thread.sleep(1); | ||
| sb.append(line).append('\n'); |
There was a problem hiding this comment.
This changes the validity contract for terminated lines. BufferedReader.readLine() can return the last partial line at EOF, and appending \n here makes checkOutput() count it as a complete line. Previously, output without an actual line terminator would remain too short.
| } | ||
| processStdin = process.getOutputStream(); | ||
| processStdout = process.getInputStream(); | ||
| processInputReader = process.inputReader(); |
There was a problem hiding this comment.
This uses the platform default charset, while the previous implementation decoded stdout with Agent.UTF8. Please preserve the existing behavior with process.inputReader(UTF8) or an explicit new InputStreamReader(process.getInputStream(), UTF8).
2 participants
Motivation
During the last contest we observed a high timeout rate, especially during league openings, the final stretch, and the rerun. Players at the top of Legend got eliminated by timeouts that were clearly caused by platform load rather than their own code.
One likely contributor is the polling loop in
RefereeAgent.getOutput(), which usedInputStream.available()to check whether data was ready to read, sleeping 1 ms between checks.available()only returns bytes already in the kernel buffer — it is not aware of bytes that are in transit or buffered elsewhere. Under server load, this may cause the loop to spin past the timeout deadline even when the bot had already responded in time, producing spurious timeouts.See also: #78 (soft-timeout approach tackling the same problem).
Changes
CompletableFuture.orTimeout— replaces theavailable()busy-wait loop with a blockingBufferedReader.readLine()offloaded to a dedicated I/O thread. The future times out precisely aftertimeoutms regardless of server load.ExecutorService(AGENT_IO_EXECUTOR) — a single daemon thread handles all blocking reads, avoidingForkJoinPoolstarvation thatCompletableFuture.supplyAsyncwith the common pool would risk.BufferedReaderinstead of raw InputStream byte-reads — simpler line-based reading replaces the manual byte-by-byte loop with CR/LF bookkeeping.processStdout/getOutputStream()— the raw InputStream field is no longer needed; only theBufferedReaderis used.RefereeAgent.getOutput()— ~50 lines of duplicated timing logic removed; now delegates toAgent.getOutput()with the appropriate buffer size limit.