feat(http): expose upstream_states and request timing/byte accessors#291
feat(http): expose upstream_states and request timing/byte accessors#291u5surf wants to merge 1 commit into
Conversation
Add safe accessors for request struct fields that previously
required raw-pointer deref:
- `Request::upstream_states() -> &[UpstreamState]` walks each
upstream attempt (one per peer for `proxy_next_upstream`
retries) via a `#[repr(transparent)]` wrapper exposing
`peer`, `status`, `response_time`, `connect_time`,
`header_time`, `queue_time`, `bytes_sent`, `bytes_received`,
`response_length`.
- `Request::start_sec()`, `start_msec()`, `request_length()`,
`bytes_sent()` for the request-level timing/byte fields.
`Request::upstream()` (the raw-pointer accessor) is left untouched
to preserve compatibility.
Unit tests follow the `MaybeUninit::zeroed` pattern from nginx#272.
Signed-off-by: Y.Horie <u5.horie@gmail.com>
2d4aceb to
0cec0b9
Compare
avahahn
left a comment
avahahn
left a comment
There was a problem hiding this comment.
Just a few minor nits. Otherwise this MR does what it says it does in a relatively safe way.
| /// Bytes sent to the client on this request's connection. | ||
| /// Reads `r->connection->sent`, the byte total nginx itself uses | ||
| /// to render `$bytes_sent` in `log_format`. | ||
| pub fn bytes_sent(&self) -> off_t { |
There was a problem hiding this comment.
We may prefer in the future to have a separate type for surfacing ngx_connection_t details to the caller so It may be best to not litter request with details of it for now, but I will defer to @bavshin-f5 on that.
| state.0.peer = &raw mut peer; | ||
|
|
||
| let got = state.peer().expect("peer should resolve"); | ||
| assert_eq!(got.as_bytes(), bytes); |
There was a problem hiding this comment.
this final assert statement is simply testing ngx_str_t.
| } | ||
| // SAFETY: `upstream_states`, when non-null, points to an | ||
| // `ngx_array_t` allocated in the request pool, valid for the | ||
| // lifetime of the request. |
There was a problem hiding this comment.
I wonder if we can use a lifetime on the returned slice to inform rust that the lifetime is equivalent to the lifetime of the self reference.
| /// uses the upstream framework to consult the cache) but no | ||
| /// backend was contacted, plus init-time slots before peer | ||
| /// selection in failure cases. | ||
| pub fn peer(&self) -> Option<&NgxStr> { |
There was a problem hiding this comment.
again here: I wonder if we can set the lifetime of the NgxStr to an identical lifetime attached to self.
2 participants
Proposed changes
Add safe accessors for request struct fields that previously required raw-pointer deref:
Request::upstream_states() -> &[UpstreamState]walks each upstream attempt (one per peer forproxy_next_upstreamretries) via a#[repr(transparent)]wrapper exposingpeer,status,response_time,connect_time,header_time,queue_time,bytes_sent,bytes_received,response_length.Request::start_sec(),start_msec(),request_length(),bytes_sent()for the request-level timing/byte fields.Request::upstream()(the raw-pointer accessor) is left untouched to preserve compatibility.Unit tests follow the
MaybeUninit::zeroedpattern from #272.Checklist
Before creating a PR, run through this checklist and mark each as complete.