Skip to content

feat(http): expose upstream_states and request timing/byte accessors - #291

Open
u5surf wants to merge 1 commit into
nginx:mainfrom
u5surf:feat/request-upstream-states-accessor
Open

u5surf wants to merge 1 commit into
nginx:mainfrom
u5surf:feat/request-upstream-states-accessor

Conversation

@u5surf

@u5surf u5surf commented May 25, 2026

Copy link
Copy Markdown
Contributor

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 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 #272.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have written my commit messages in the Conventional Commits format.
  • I have read the CONTRIBUTING doc
  • I have added tests (when possible) that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@u5surf
u5surf force-pushed the feat/request-upstream-states-accessor branch from 2d4aceb to 0cec0b9 Compare May 25, 2026 10:25

@avahahn avahahn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nits. Otherwise this MR does what it says it does in a relatively safe way.

Comment thread src/http/request.rs
/// 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/http/upstream.rs Outdated
state.0.peer = &raw mut peer;

let got = state.peer().expect("peer should resolve");
assert_eq!(got.as_bytes(), bytes);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this final assert statement is simply testing ngx_str_t.

Comment thread src/http/request.rs
}
// SAFETY: `upstream_states`, when non-null, points to an
// `ngx_array_t` allocated in the request pool, valid for the
// lifetime of the request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/http/upstream.rs
/// 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> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again here: I wonder if we can set the lifetime of the NgxStr to an identical lifetime attached to self.

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>
@u5surf
u5surf force-pushed the feat/request-upstream-states-accessor branch from 0cec0b9 to 3a2956b Compare September 15, 2026 02:05
@u5surf

u5surf commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, and sorry for the delay. Rebased onto main and addressed the nits.

upstream.rs test assert — dropped; the test now just checks that peer() resolves a populated ngx_str_t, without re-asserting what NgxStr already covers.

Lifetimes on the returned slice and NgxStr — these are already tied to &self by elision. fn upstream_states(&self) -> &[UpstreamState] desugars to fn upstream_states<'a>(&'a self) -> &'a [UpstreamState], and the same holds for UpstreamState::peer. Writing them out explicitly makes clippy ask for them back:

warning: the following explicit lifetimes could be elided: 'a
   --> src/http/request.rs:236:28
    = note: `#[warn(clippy::needless_lifetimes)]` on by default

which would fail CI under RUSTFLAGS: --deny warnings. So I have left them elided. Happy to add a doc note spelling the borrow out instead, if that would read better.

ngx_connection_t details on Request — leaving as is pending @bavshin-f5, since you deferred that one. Easy to move behind a separate type later if that is the direction.

cargo fmt, clippy --all-targets and the test suite are clean on nginx 1.30.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants