Send the light to the browser before writing it to the spooler - #422
Merged
Conversation
A [test] run did three things in order: call runner, write the event, respond. The write sat on the browser's critical path, so its cost was added to every traffic-light the user waited for. That cost is negligible when the spooler is healthy: it acks a buffered append in about a millisecond, against a runner call of several hundred. The reason to move it is the bad case. An unreachable or hung spooler costs Net::HTTP's default 60-second connect and read timeouts, and the light waited behind that even though the runner had already produced it. The write now runs as the response body closes. An AfterResponse middleware wraps the mounted apps; a route stashes work and BodyProxy runs it once the body has gone out. The work stays inside the request, so no thread is orphaned and Puma drains it at shutdown. Wrapping outside Sinatra preserves content-length, which Sinatra computes from the route's own Array body. Deferring it first needed the http object to stop being shared. Requester held one Net::HTTP for the life of the process, and #request mutates its socket and started flags, so a deferred POST could overlap the same tab's next one. Each request now builds its own object, which costs a single allocation: the object is never started, so every request already opened and closed its own connection. Measured at 0.9ms saved from an 805ms run. Nobody will feel that; the tail is what this buys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.rubocop.yml grandfathers today's offences file-by-file in .rubocop_todo.yml so that a newly added file is linted in full from the moment it arrives. after_response.rb arrived without the frozen string literal comment every cop-clean file needs, so it is added here rather than given a todo entry, which would work against that arrangement. Two offences in kata_app.rb go with it. The comment above the deferred write ran a character over the 80-column ceiling, and is reflowed. The begin/end wrapping the write is dropped in favour of the block's own rescue, which reads the same and is what Style/RedundantBegin asks for. Only the second of those changes code rather than text, so the server suite was rerun: 132 tests, 481 assertions, no failures. It covers the rescue path directly, in run_tests_save_error_test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
A [test] run did three things in order: call runner, write the event,
respond. The write sat on the browser's critical path, so its cost was
added to every traffic-light the user waited for.
That cost is negligible when the spooler is healthy: it acks a buffered
append in about a millisecond, against a runner call of several hundred.
The reason to move it is the bad case. An unreachable or hung spooler
costs Net::HTTP's default 60-second connect and read timeouts, and the
light waited behind that even though the runner had already produced it.
The write now runs as the response body closes. An AfterResponse
middleware wraps the mounted apps; a route stashes work and BodyProxy
runs it once the body has gone out. The work stays inside the request,
so no thread is orphaned and Puma drains it at shutdown. Wrapping
outside Sinatra preserves content-length, which Sinatra computes from
the route's own Array body.
Deferring it first needed the http object to stop being shared.
Requester held one Net::HTTP for the life of the process, and #request
mutates its socket and started flags, so a deferred POST could overlap
the same tab's next one. Each request now builds its own object, which
costs a single allocation: the object is never started, so every
request already opened and closed its own connection.
Measured at 0.9ms saved from an 805ms run. Nobody will feel that; the
tail is what this buys.
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com