feat(httpserver): add request lifecycle signals - #158
Conversation
There was a problem hiding this comment.
Clean addition — the three-signal design covers the full request lifecycle well, the try/except/else nesting in _handle_connection is correct, and the 14 tests are thorough (mock writers, streaming variants, ordering, exception suppression, timing verification). CI green 3.10–3.13.
1. on_response_started doesn't receive response
The signal fires after _dispatch() returns and before _write() — the response object exists at this point. For TTFB metrics you often want the status code / content-type alongside timing:
@app.on_response_started
async def ttfb(request):
# Can't log response.status_code here
request.state.response_start = time.monotonic()You can work around it via on_response_completed, but then the timestamp is post-transfer, not pre-write. Passing (request, response) to on_response_started would close the gap — same signature as on_response_completed and no cost since the object already exists.
2. StreamingResponse._write() re-raise changes the method's contract
Previously _write() silently caught disconnect errors; now it re-raises. This is the right design (centralizes disconnect handling in _handle_connection), but it changes the contract of a method that has a clear "write and handle errors internally" pattern. Worth a brief docstring note on _write() that callers must handle BrokenPipeError / ConnectionResetError / ConnectionAbortedError, since _write is used in only one place but defines the boundary between response serialization and connection lifecycle.
3. on_client_disconnect doesn't fire for dispatch-phase disconnects
If the client disconnects while _dispatch() is running (e.g., during a slow handler that reads from the request body), the resulting exception propagates to the outer except Exception — on_client_disconnect never fires. The signal only covers write-phase disconnects. This is probably the correct scope (naming says "client disconnect," not "connection lost"), but documenting it in the docstring ("fired when the client disconnects during response delivery") would set expectations.
4. Incidental manifest.json diff
The jsonschema module's content_hash and last_updated changed in this PR's manifest. Looks like a rebase artifact from master. Not blocking but adds noise — could be cleaned up with a regenerate on the final merge commit.
Non-blocking
- Hook iteration is sequential — three
for hook in handlersloops run one at a time. Fine for the common case (1-2 lightweight hooks), but a slow sync hook inon_response_starteddelays the actual response write. A note in the docstring ("handlers run sequentially; keep them fast") would help. on_response_completedfires before thefinallyblock'swriter.close()— so the response is "completed" from the server's perspective, but the TCP connection isn't fully torn down yet. Correct behavior, just noting it's intentional.
|
Addressed all points in 992e485:
|
There was a problem hiding this comment.
Review — feat(httpserver): add request lifecycle signals
Clean addition. The three-signal model (on_response_started / on_response_completed / on_client_disconnect) maps well to the ASGI lifecycle, the StreamingResponse._write re-raise is the right structural fix, and the test suite is thorough (14 tests covering sync/async, exception suppression, ordering, streaming, mid-stream disconnect, timing, background tasks). LGTM with two items to consider:
1. on_response_started could receive response — signature lock-in concern
The response object is already materialized when the signal fires (after _dispatch, before _write). Right now the handler gets (request) only, but ASGI's equivalent http.response.start includes status and headers. Users wanting TTFB-by-status-code metrics would need both on_response_started and on_response_completed to correlate the start timestamp with the status code.
Since this is a public API and changing (request) → (request, response) later is a breaking change, worth considering the two-arg signature now:
for hook in self._on_response_started_handlers:
try:
await _invoke(hook, request, response) # response already available
except Exception:
logger.warning("on_response_started hook failed", exc_info=True)Non-blocking — the current signature is coherent and documented; this is a forward-compatibility consideration.
2. BrokenPipeError during _dispatch now escalates from DEBUG → ERROR
The old structure caught (BrokenPipeError, ConnectionResetError, ConnectionAbortedError) around the combined _dispatch + _write block. The refactored structure only catches them around _write, so if _dispatch raises one of these (e.g. a before_request hook doing external IO), it falls through to the outer except Exception → logger.exception(...) — a full ERROR traceback instead of the quiet logger.debug("Connection reset by %s ...").
In practice this is unlikely since _dispatch doesn't touch the writer, but it is a behavioral change. Easy fix if desired — wrap _dispatch in its own guard:
try:
response = await self._dispatch(request)
except (BrokenPipeError, ConnectionResetError, ConnectionAbortedError):
logger.debug("Connection reset by %s during dispatch", client_addr)
returnWhat looks good:
StreamingResponse._writere-raise is clean —finallystill runsaclose(), and the only caller (_handle_connection) now catches properly_invokewithasyncio.to_threadfor sync handlers is correct —awaitpreserves ordering guarantees- Exception suppression with per-hook
try/except Exception+logger.warningis the right pattern (one failing hook doesn't block others or crash the response) - Test coverage is excellent — the
_MidStreamDisconnectWriter(succeeds on headers, fails on body) is a nice touch for verifying bothon_client_disconnectandbackgroundfire on partial streaming failures - Version bump 0.4.0 → 0.5.0 is appropriate for new public API surface
There was a problem hiding this comment.
Clean addition — the three-signal model maps well to the request lifecycle, the StreamingResponse._write re-raise fixes a genuine inconsistency (streaming disconnects were silently swallowed while non-streaming ones propagated), and the 14 tests are thorough. CI green across the board. LGTM.
1. Extract a _fire_signal helper
The three signal-firing loops in _handle_connection are nearly identical — iterate handlers, _invoke, try/except Exception, logger.warning. A small helper would cut ~15 lines and keep the semantics in one place:
async def _fire_signal(self, name: str, handlers: list, *args) -> None:
for hook in handlers:
try:
await _invoke(hook, *args)
except Exception:
logger.warning("%s hook failed", name, exc_info=True)Then the calling code becomes three one-liners:
await self._fire_signal("on_response_started", self._on_response_started_handlers, request)
await self._fire_signal("on_response_completed", self._on_response_completed_handlers, request, response)
await self._fire_signal("on_client_disconnect", self._on_client_disconnect_handlers, request)2. +1 on passing response to on_response_started
Both Clementine and Elena flagged this — the response object exists, the signature would become symmetric with on_response_completed, and it avoids lock-in on a public API. Agree this is worth doing now.
3. Document signal ordering relative to middleware
Users will want to know where lifecycle signals sit in the existing chain. A one-line note in the class docstring would preempt the question:
before_request → route handler → after_request → on_response_started → write → on_response_completed
↘ on_client_disconnect
Non-blocking
- Elena's dispatch-phase disconnect point is good —
_dispatchraisingBrokenPipeErrornow falls through to the genericexcept Exception(ERROR-level traceback). Wrapping_dispatchin its own(BrokenPipeError, ConnectionResetError, ConnectionAbortedError)guard or adding it to the existing catch would preserve the quiet DEBUG log. - Manifest noise — the jsonschema
content_hash/last_updateddiff is a rebase artifact. Regenerating on merge would clean it up.
|
Addressed Elena's feedback in 7ac3101:
|
|
Addressed Milo's remaining feedback in 8c44f93:
All 109 tests pass, pre-commit clean. |
Add on_response_started, on_response_completed, and on_client_disconnect signal decorators for fine-grained request lifecycle observability. - on_response_started: fires before response write (TTFB metrics) - on_response_completed: fires after successful response delivery - on_client_disconnect: fires on broken pipe / connection reset All signals support sync/async handlers, suppress exceptions, and work with both regular and streaming responses. StreamingResponse._write() now re-raises disconnect errors so _handle_connection can fire the on_client_disconnect signal. Closes #136
- on_response_started now receives (request, response) so handlers can inspect status code / content-type alongside TTFB timing - Document StreamingResponse._write() re-raise contract in docstring - Scope on_client_disconnect docstring to response-delivery phase - Add "handlers run sequentially" note to signal docstrings - Regenerate manifest.json to remove rebase noise
Wrap _dispatch() in its own BrokenPipeError/ConnectionResetError guard so a disconnect during dispatch logs at DEBUG level (matching prior behavior) instead of falling through to the outer except Exception which logs a full ERROR traceback.
…iagram - Add _fire_signal() method to deduplicate the three signal-firing loops in _handle_connection (addresses Milo's review feedback) - Add request lifecycle diagram to App class docstring showing signal ordering relative to middleware
8c44f93 to
72a90d2
Compare
Summary
on_response_started,on_response_completed, andon_client_disconnectsignal decorators toAppfor fine-grained request lifecycle observabilityon_response_startedfires before response write — useful for TTFB metricson_response_completedfires after successful response delivery — useful for total transfer timeon_client_disconnectfires on broken pipe / connection reset — useful for cleanup and metricsStreamingResponse._write()now re-raises disconnect errors so_handle_connectioncan fireon_client_disconnectCloses #136
Test plan
make test-httpserver— 109 passed, 0 failed (95 existing + 14 new)