Filedrop

TcpServer admission race - analysis and two measured fixes

1 file(s) · 18 KB · Created 1d ago · Expires in 5d 17h

Files

tcpserver-admission-race-standalone.html18 KB text
<!doctype html> <html lang="en"> <head> <meta charset="utf-8"> <meta name="viewport" content="width=device-width, initial-scale=1"> <style> *, *::before, *::after { box-sizing: border-box; } * { margin: 0; } html { -webkit-text-size-adjust: 100%; } img, svg { display: block; max-width: 100%; } h1, h2, h3, p, ul, ol, pre, table { margin: 0; } </style> <title>TcpServer admission race — analysis and two measured fixes</title> <style> :root { --ground: #F4F5F3; --surface: #FCFCFB; --ink: #191C22; --muted: #5B6068; --faint: #878C93; --rule: #DCDFDA; --rule-soft:#E9EBE7; --accent: #9A5B00; --accent-bg:#F2E7D6; --good: #2E6B45; --bad: #9C2B2B; --code-bg: #EDEFEB; --serif: "Iowan Old Style", "Palatino Linotype", Palatino, Georgia, serif; --sans: ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, sans-serif; --mono: ui-monospace, SFMono-Regular, "SF Mono", Menlo, Consolas, monospace; --measure: 68ch; } @media (prefers-color-scheme: dark) { :root { --ground: #14161A; --surface: #1B1E23; --ink: #E7E9E4; --muted: #A2A8AF; --faint: #7C828A; --rule: #2E333A; --rule-soft:#23272D; --accent: #E0A040; --accent-bg:#33281A; --good: #6FBF8E; --bad: #E38A8A; --code-bg: #1F232A; } } :root[data-theme="dark"] { --ground: #14161A; --surface: #1B1E23; --ink: #E7E9E4; --muted: #A2A8AF; --faint: #7C828A; --rule: #2E333A; --rule-soft:#23272D; --accent: #E0A040; --accent-bg:#33281A; --good: #6FBF8E; --bad: #E38A8A; --code-bg: #1F232A; } :root[data-theme="light"] { --ground: #F4F5F3; --surface: #FCFCFB; --ink: #191C22; --muted: #5B6068; --faint: #878C93; --rule: #DCDFDA; --rule-soft:#E9EBE7; --accent: #9A5B00; --accent-bg:#F2E7D6; --good: #2E6B45; --bad: #9C2B2B; --code-bg: #EDEFEB; } body { background: var(--ground); color: var(--ink); font-family: var(--sans); font-size: 17px; line-height: 1.65; -webkit-font-smoothing: antialiased; } .page { max-width: 78ch; margin: 0 auto; padding: clamp(2rem, 5vw, 4.5rem) clamp(1.1rem, 4vw, 2rem) 6rem; display: flex; flex-direction: column; gap: 2.5rem; } .masthead { display: flex; flex-direction: column; gap: 0.9rem; } .eyebrow { font-family: var(--mono); font-size: 0.72rem; letter-spacing: 0.13em; text-transform: uppercase; color: var(--accent); } h1 { font-family: var(--serif); font-size: clamp(1.85rem, 4.6vw, 2.7rem); line-height: 1.14; font-weight: 600; letter-spacing: -0.012em; text-wrap: balance; max-width: 24ch; } .standfirst { font-family: var(--serif); font-size: 1.12rem; line-height: 1.55; color: var(--muted); max-width: var(--measure); } .meta { font-family: var(--mono); font-size: 0.74rem; color: var(--faint); display: flex; flex-wrap: wrap; gap: 0.4rem 1.4rem; padding-top: 0.7rem; border-top: 1px solid var(--rule); } section { display: flex; flex-direction: column; gap: 1rem; } h2 { font-family: var(--serif); font-size: 1.42rem; font-weight: 600; line-height: 1.25; letter-spacing: -0.006em; text-wrap: balance; padding-bottom: 0.5rem; border-bottom: 1px solid var(--rule); } h3 { font-family: var(--sans); font-size: 0.78rem; font-weight: 650; letter-spacing: 0.09em; text-transform: uppercase; color: var(--muted); margin-top: 0.5rem; } p { max-width: var(--measure); } p + p { margin-top: 0.15rem; } a { color: var(--accent); text-underline-offset: 0.16em; } a:focus-visible, summary:focus-visible { outline: 2px solid var(--accent); outline-offset: 3px; border-radius: 2px; } code { font-family: var(--mono); font-size: 0.86em; background: var(--code-bg); padding: 0.1em 0.34em; border-radius: 3px; } pre { background: var(--code-bg); border: 1px solid var(--rule-soft); border-radius: 4px; padding: 1rem 1.1rem; overflow-x: auto; font-size: 0.83rem; line-height: 1.6; } pre code { background: none; padding: 0; font-size: 1em; } ul, ol { max-width: var(--measure); padding-left: 1.3rem; display: flex; flex-direction: column; gap: 0.45rem; } li::marker { color: var(--faint); } /* Ordered race timeline — the numbering encodes real happens-before order. */ .timeline { display: flex; flex-direction: column; gap: 0; margin: 0.4rem 0; border-left: 2px solid var(--rule); } .step { display: grid; grid-template-columns: 2.6rem 1fr; gap: 0.9rem; padding: 0.72rem 0 0.72rem 1.1rem; position: relative; } .step + .step { border-top: 1px solid var(--rule-soft); } .step-n { font-family: var(--mono); font-size: 0.72rem; color: var(--faint); padding-top: 0.18rem; font-variant-numeric: tabular-nums; } .step-actor { font-family: var(--mono); font-size: 0.73rem; color: var(--accent); display: block; margin-bottom: 0.1rem; } .step.window { background: var(--accent-bg); border-left: 2px solid var(--accent); margin-left: -2px; } .step.window .step-n { color: var(--accent); font-weight: 700; } .table-wrap { overflow-x: auto; border: 1px solid var(--rule); border-radius: 4px; background: var(--surface); } table { border-collapse: collapse; width: 100%; font-size: 0.92rem; } th, td { text-align: left; padding: 0.7rem 0.95rem; border-bottom: 1px solid var(--rule-soft); vertical-align: top; } thead th { font-family: var(--sans); font-size: 0.7rem; letter-spacing: 0.08em; text-transform: uppercase; color: var(--muted); font-weight: 650; border-bottom: 1px solid var(--rule); white-space: nowrap; } tbody tr:last-child td { border-bottom: none; } .num { font-family: var(--mono); font-variant-numeric: tabular-nums; white-space: nowrap; } .fail { color: var(--bad); font-weight: 650; } .pass { color: var(--good); font-weight: 650; } .callout { background: var(--surface); border: 1px solid var(--rule); border-left: 3px solid var(--accent); border-radius: 4px; padding: 1.05rem 1.2rem; display: flex; flex-direction: column; gap: 0.5rem; } .callout h3 { margin-top: 0; color: var(--accent); } .callout p { max-width: none; } .ask { border-left-color: var(--muted); } .ask h3 { color: var(--muted); } ol.questions { counter-reset: q; list-style: none; padding-left: 0; gap: 0.9rem; } ol.questions > li { display: grid; grid-template-columns: 1.9rem 1fr; gap: 0.7rem; } ol.questions > li::before { counter-increment: q; content: "Q" counter(q); font-family: var(--mono); font-size: 0.72rem; color: var(--accent); padding-top: 0.24rem; font-weight: 700; } footer { border-top: 1px solid var(--rule); padding-top: 1.1rem; font-size: 0.85rem; color: var(--faint); max-width: var(--measure); } @media (prefers-reduced-motion: reduce) { * { animation: none !important; transition: none !important; } } </style> </head> <body> <div class="page"> <header class="masthead"> <span class="eyebrow">UrlProtocol · TcpServer · request for second opinion</span> <h1>A server that refuses capacity nothing is using</h1> <p class="standfirst"> <code>TcpServer</code> reports its concurrent-handler limit to callers that are within that limit. The cause is settled and reproducible. What I want challenged is the fix: closing it completely costs a platform-thread bound that was deliberately established one commit earlier, and I believe that trade is unavoidable rather than a failure of imagination. That belief is the thing to attack. </p> <div class="meta"> <span>Repo: CodexCoder21Organization/UrlProtocol</span> <span>Base: e804219c</span> <span>Defect arrived in: PR #493</span> </div> </header> <section> <h2>What is observably wrong</h2> <p> A caller that completes one request and immediately issues the next receives: </p> <pre><code>RpcError(code=CONCURRENT_HANDLER_LIMIT, message=TcpServer on port 41571 is at its concurrent request-handler limit of 2; request recovered-request for method 'status' was not started)</code></pre> <p> At the moment of that refusal, no handler is running. The statement is false, and it is user-visible: the caller is told to back off by a server that is idle. </p> <p> This surfaced as a red <code>kotlin.build (remote)</code> check — <code>testTcpServerRejectsRequestsAtConcurrentHandlerLimitAndRecovers</code>, 1 failure in 1372 tests, retries exhausted. It passes on an unloaded local machine and fails on the loaded CI droplet, which is the signature of a timing window rather than a broken assertion. </p> </section> <section> <h2>Mechanism</h2> <p> Admission was never counted. It was <em>inferred</em> from a thread pool: </p> <pre><code>ThreadPoolExecutor( 0, // corePoolSize concurrentHandlerLimit, // maximumPoolSize REQUEST_THREAD_KEEP_ALIVE_SECONDS, TimeUnit.SECONDS, SynchronousQueue(), threadFactory, ThreadPoolExecutor.AbortPolicy(), )</code></pre> <p> A <code>SynchronousQueue</code> has no capacity of its own. <code>execute()</code> succeeds only if it can hand the task to a worker <strong>currently parked in <code>poll()</code></strong>, or if it may create a new thread. Once <code>poolSize</code> has reached <code>maximumPoolSize</code>, the second option is gone — so capacity means <em>“a worker is parked right now”</em>, not <em>“a handler slot is free”</em>. </p> <p> Those two things diverge for as long as it takes the scheduler to run a worker that has just finished. Ordered by happens-before: </p> <div class="timeline"> <div class="step"> <span class="step-n">1</span> <div><span class="step-actor">reader thread</span>Reads request <em>i</em>, calls <code>execute()</code>. A worker takes it.</div> </div> <div class="step"> <span class="step-n">2</span> <div><span class="step-actor">worker</span>Runs the handler and writes the response to the socket.</div> </div> <div class="step window"> <span class="step-n">3</span> <div><span class="step-actor">worker — the window</span>Returns from the task and begins unwinding toward <code>poll()</code>. It is not yet parked, and <code>poolSize == maximumPoolSize</code>.</div> </div> <div class="step window"> <span class="step-n">4</span> <div><span class="step-actor">client</span>Has already read the response from step 2. Sends request <em>i+1</em>.</div> </div> <div class="step window"> <span class="step-n">5</span> <div><span class="step-actor">reader thread</span><code>execute()</code> finds no parked worker and may not create one. <code>RejectedExecutionException</code> → <code>CONCURRENT_HANDLER_LIMIT</code>.</div> </div> <div class="step"> <span class="step-n">6</span> <div><span class="step-actor">worker</span>Finally parks. Capacity “returns”, having never actually been used.</div> </div> </div> <p> Steps 3–5 are the defect. CPU contention widens the gap between 3 and 6, which is exactly why it reproduces on a loaded CI droplet and hides on an idle laptop. </p> </section> <section> <h2>Reproducer</h2> <p> The assertion is designed to be machine-independent. <em>N</em> client lanes each issue requests strictly sequentially, waiting for each response before sending the next, and there are exactly <code>concurrentHandlerLimit</code> lanes. At most <code>concurrentHandlerLimit</code> requests can be in flight at any instant, so the server is never genuinely saturated and <strong>no</strong> response may carry <code>CONCURRENT_HANDLER_LIMIT</code>. One such response is a failure. </p> <p> The iteration count samples the window; it is not load, and it is not a threshold. Nothing is tuned to a machine — a faster host changes how often step 4 lands inside steps 3–6, not whether the assertion is correct. </p> <div class="callout"> <h3>Why the failure distribution confirms the mechanism</h3> <p> On unfixed <code>main</code>, <strong>every lane failed at request index 1</strong> — the very first request after its first response. Request 0 creates the worker; the worker finishes and writes; request 1 arrives before it has re-parked. That is step 3 exactly, and it is the reason I stopped looking for a load explanation. </p> </div> </section> <section> <h2>Two variants, measured</h2> <p> Both replace the inferred capacity with an explicit <code>Semaphore(concurrentHandlerLimit)</code> acquired before dispatch, and let the pool grow unbounded so it can never reject an admitted request. They differ only in <em>when the permit is released</em>. Same reproducer, same host, 1600 requests. </p> <div class="table-wrap"> <table> <thead> <tr> <th>Variant</th> <th>Permit released</th> <th class="num">Spurious refusals</th> <th>PR #493 test</th> </tr> </thead> <tbody> <tr> <td>Unfixed <code>main</code></td> <td>n/a — capacity inferred from the pool</td> <td class="num fail">23 / 1600</td> <td class="fail">fails on CI</td> </tr> <tr> <td>A — semaphore</td> <td>after the response is written</td> <td class="num fail">1 / 1600</td> <td class="pass">passes</td> </tr> <tr> <td>B — semaphore</td> <td>after the handler returns, before the write</td> <td class="num pass">0 / 1600</td> <td class="pass">passes</td> </tr> </tbody> </table> </div> <p> Variant A removes the scheduler from the accounting and fixes the reported CI failure, but leaves a narrower instance of the same class: the client reads the response before the handler thread executes <code>release()</code>. Variant B releases after the handler returns and before any byte can reach the caller, so a caller holding a response is guaranteed the slot that served it is already free. </p> </section> <section> <h2>The trade, and why I think it is inherent</h2> <p> This is the part I most want a second opinion on, because the argument is short enough to be suspicious. </p> <div class="callout"> <h3>The argument</h3> <p> For the server never to refuse a caller that respects the limit, nothing that gates admission may still be held while the response is being written — otherwise a caller holding its response can still be refused, which is the bug. But the <em>thread</em> is held through the write, by construction. Therefore threads cannot be the gate. </p> </div> <p> If that holds, variant B's cost is not an oversight but the price: with the permit released before the write, a peer that pipelines requests while not reading its responses is no longer refused, and its handlers accumulate threads instead — one writing, the rest blocked on the connection's <code>output</code> monitor. Previously the held slot supplied that backpressure. That backpressure is what PR #493 established one commit earlier, specifically to bound platform threads. </p> <p> My reading is that bounding a stalled peer belongs at the socket layer — per-connection in-flight response limits, or write deadlines — not in handler-slot accounting, and that refusing unrelated well-behaved callers is the wrong way to get it. But I hold that loosely, and a redesign that bounds writes properly would dominate both variants. </p> <div class="callout"> <h3>Evidence that partly cuts against my own concern</h3> <p> Under variant B all 26 existing TCP tests pass, including <code>testTcpPushIsBoundedWhenClientStopsReading</code> — the repository's own test for the stalled-reader case I am worried about. So the concern is not currently demonstrated by any test in the suite. Either the existing coverage does not reach the accumulation path, or the path is bounded by something I have not identified. I could not tell which, and that uncertainty is why this document exists rather than just a merged PR. </p> </div> </section> <section> <h2>What I would like attacked</h2> <ol class="questions"> <li>Is the inherency argument actually sound, or is there a formulation where admission is gated by something released before the write while threads stay bounded — a bounded writer stage, non-blocking writes, or per-connection in-flight caps?</li> <li>Is “a caller within the limit is never refused” the right contract at all? The alternative is that a slot is occupied until its response is fully written, which makes variant A correct and makes my stricter reproducer wrong rather than the server.</li> <li>Variant B changes behaviour under a hostile or stalled peer, in a subsystem hardened for exactly that one commit earlier. Is shipping it without the socket-layer bound in place too risky to do in one step?</li> <li>Is 1600 requests on one host adequate evidence that B is 0 rather than merely rarer than A? I did not run a power analysis, and A's residual was 1 in 1600.</li> </ol> </section> <footer> Measurements are from a single 16-core host, one run per variant, using the same reproducer and the same <code>concurrentHandlerLimit = 4</code> with 4 lanes × 400 sequential requests. The CI observation is from buildtest run <code>a44e4416</code>, read from the executor's <code>test-events.jsonl</code> rather than the frontend, which was returning 500/503 at the time. </footer> </div> </body> </html>

Upload Files

Drop files here or click to browse