feat: expose stub_status connection-state atomics via event::connecti… - #290
feat: expose stub_status connection-state atomics via event::connecti…#290u5surf wants to merge 1 commit into
Conversation
5f74c2c to
39e7100
Compare
avahahn
left a comment
avahahn
left a comment
There was a problem hiding this comment.
This looks good but I think just a few alterations are in order.
| @@ -0,0 +1,161 @@ | |||
| //! Helpers around nginx's event-loop globals. | |||
|
|
|||
| #[cfg(ngx_feature = "stat_stub")] | |||
There was a problem hiding this comment.
I am not sure this needs a feature gate, but ill defer to @bavshin-f5 or @ensh63 if either of them agree with having it.
There was a problem hiding this comment.
@avahahn
Thanks for the review — ②③④ are addressed in the force-pushed commit (rebased onto main, squashed per CONTRIBUTING).
On the gate: it's needed, but not as a Cargo feature — ngx_feature is the build-time cfg nginx-sys derives from nginx's own ngx_auto_config.h (NGX_STAT_STUB → cargo::rustc-cfg=ngx_feature="stat_stub"). nginx declares these externs under #if (NGX_STAT_STUB) (src/event/ngx_event.h:468), so without stub_status bindgen emits nothing and the module doesn't compile:
error[E0432]: unresolved imports `crate::ffi::ngx_stat_accepted`,
`crate::ffi::ngx_stat_active`, `crate::ffi::ngx_stat_handled`,
`crate::ffi::ngx_stat_reading`, `crate::ffi::ngx_stat_requests`,
`crate::ffi::ngx_stat_waiting`, `crate::ffi::ngx_stat_writing`
error: could not compile `ngx` (lib) due to 1 previous error
(cargo build -p ngx --features vendored with the cfg attributes removed.)
Worth noting separately: NGINX_CONFIGURE_BASE has no --with-http_stub_status_module, so CI never compiles this path — I verified locally with NGX_CONFIGURE_ARGS="--with-http_stub_status_module" (tests + clippy pass with and without, nginx 1.30.4). Happy to add the flag here or leave it for a separate PR.
@bavshin-f5 @ensh63 — flagging in case you see it differently.
| pub use connection_stats::{ConnectionStats, connection_stats}; | ||
|
|
||
| #[cfg(ngx_feature = "stat_stub")] | ||
| mod connection_stats { |
There was a problem hiding this comment.
If we add more features to this src/event.rs file (which we very well may in the future) then having many features as nested mod blocks will mean that the tests will be interleaved through the code as well as many more use statements that could be de-duplicated and in one place. I think that will make it harder to read through and navigate the code. Lets not bother with the mod here even though we have used them elsewhere in this project.
| } | ||
|
|
||
| #[test] | ||
| #[allow(clippy::unnecessary_cast)] // `ngx_atomic_t::MAX as u64` is a no-op on 64-bit targets. |
There was a problem hiding this comment.
This test will require hardware that has a non 64 bit addressing bus in order to actually test anything.
Its not impossible to do in github CI but it is a bunch of work and isnt implemented here. Also this doesn't test any logic that NGINX or ngx-rust implements.
Lets just pull that test out for now?
| use super::*; | ||
|
|
||
| #[test] | ||
| fn default_is_all_zero() { |
There was a problem hiding this comment.
Since default is derived on this type your test is only testing the logic of the Default trait derive macro. I don't see us ever choosing to implement an alternative default so I think there is no need for this test.
39e7100 to
590d4e1
Compare
…on_stats Add a safe accessor for the seven `ngx_stat_*` globals nginx populates when built with `--with-http_stub_status_module`. Bindgen already emits the raw externs once that build flag is set, but consumers currently have to read them by hand with several `unsafe` casts; the new `event::connection_stats()` function returns a `ConnectionStats` snapshot in one call. Gated behind `#[cfg(ngx_feature = "stat_stub")]` so nginx builds without stub_status still link cleanly. Signed-off-by: Y.Horie <u5.horie@gmail.com>
590d4e1 to
7f91843
Compare
2 participants
…on_stats
Proposed changes
Describe the use case and detail of the change. If this PR addresses an issue
on GitHub, make sure to include a link to that issue here in this description
(not in the title of the PR).
We would like to pure rust nginx status module, so we’d like to have a safe accessor for the seven
ngx_stat_*globals nginx populates when built with--with-http_stub_status_module.We can already emits the raw externs once that build flag is set, but we currently have to read them by hand with several
unsafecasts; the newevent::connection_stats()function returns aConnectionStatssnapshot in one call.Checklist
Before creating a PR, run through this checklist and mark each as complete.