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
…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>
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.
| 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.
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.