anng: update anng to use nng-sys 0.4.0-v2pre.1 (NNG 2.0)#37
anng: update anng to use nng-sys 0.4.0-v2pre.1 (NNG 2.0)#37flxo merged 37 commits intonanomsg:mainfrom
Conversation
ca90672 to
779d4f2
Compare
1fcb297 to
64f316d
Compare
Major update to support NNG v2.0: API changes: - Use typed nng_err enum instead of raw u32 error constants - Rename socket functions: nng_close → nng_socket_close, nng_send_aio → nng_socket_send, nng_recv_aio → nng_socket_recv - Update nng_strerror signature to take nng_err enum - Add NNG initialization via nng_init() (required by NNG 2.0) Dependency changes: - Update nng-sys from 0.3.0 to 0.4.0-v2pre.1 - Remove build.rs version detection (no longer needed) - Remove #[cfg(nng_110)] gates as NNG 2.0 is now baseline Error handling: - Use nng_err::NNG_* variants with pattern guards - Format transport errors directly (NNG doesn't resolve them) - Consistent 'err' naming in match patterns Test updates: - Adjust error kind expectations for NNG 2 behavior - Update inproc address format expectations - Enable pubsub tests unconditionally
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@anng/Cargo.toml`:
- Around line 30-31: The Cargo.toml for crate "anng" references a non-published
nng-sys via version = "0.4.0-v2pre.1" and path = "../nng-sys"; resolve this by
either publishing nng-sys with that exact version and keeping the path override
removed, or revert the path = "../nng-sys" override and update the version =
"0.4.0-v2pre.1" value to a version string that is actually published on
crates.io for nng-sys so Cargo can resolve the dependency when publishing anng.
In `@anng/src/aio.rs`:
- Around line 340-345: The AioError::Operation branch must not transmute the raw
u32 into nng_err (unsound for flagged codes); instead reuse the existing
From<AioError> for io::Error logic that safely unpacks flag bits (the
NNG_ESYSERR/NNG_ETRANERR masks) to obtain the base error value, then produce the
message—either by converting the AioError to an io::Error (using
io::Error::from) and formatting that, or by applying the same bit-unpacking used
in the From<AioError> for io::Error impl before calling crate::nng_strerror;
update the AioError::Operation arm (the code handling AioError::Operation and
the call to crate::nng_strerror) to use that safe path instead of
core::mem::transmute.
🧹 Nitpick comments (1)
anng/src/message.rs (1)
395-423: Confirmnng_pipe_get_addrerror surface in NNG 2.0.If
NNG_ETRANERR(or other non-ESYSERR errors) can be returned here, consider handling it similarly to the ESYSERR path instead ofunreachable!.
|
On the question of "what should we store in |
Add init_nng() function to configure NNG thread pools before socket creation: - NngConfig with task_threads, expire_threads, poller_threads, num_resolver_threads - ThreadPoolConfig with num and max (ThreadLimit::Unlimited or Limit) - Validates num <= max constraint before calling nng_init - InitError::Invalid for bad config, AlreadyInitialized if config provided after init Automatic initialization with defaults when first socket is created. init_nng(None) can be called multiple times safely.
Ok. I posted a query on discord about the return values. Think we can decide how to process if there is a confirmation that all ints are are actually |
Replace NonZeroUsize with NonZeroI16 in ThreadPoolConfig and related types. This makes invalid values unrepresentable at the type level, eliminating the need for runtime overflow validation. Addresses review comment: nanomsg#37 (comment)
Add deinit_nng() function that wraps nng_fini() to allow explicit cleanup of NNG library resources. This is not auto-called; users must ensure all NNG resources are closed before calling. Addresses review comment: nanomsg#37 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@anng/src/init.rs`:
- Around line 124-136: The function deinit_nng currently has a safe signature
but relies on the caller to ensure no NNG resources remain, so change its
signature to pub unsafe fn deinit_nng() and update the doc comment to state the
function is unsafe and must only be called when all NNG
sockets/dialers/listeners are closed; keep the existing SAFETY comment and any
references (e.g., init_nng) intact so callers see the safety contract.
♻️ Duplicate comments (1)
anng/src/init.rs (1)
7-55: Consider#[non_exhaustive]on public init types.This would allow adding new fields/variants later without breaking downstream pattern matches.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@anng/src/init.rs`:
- Around line 139-165: The validation currently allows negative NonZeroI16
values to reach nng_init; update validate_pool_config to reject non-positive
values by checking pool.num.get() and pool.max (when Limit) are > 0 and return
InitError::Invalid if not, and add a similar positive check for
num_resolver_threads in init_nng (or a small helper used by
pool_num_to_i16/pool_max_to_i16) before building nng_init_params so num, max,
and num_resolver_threads are strictly positive.
- Around line 85-121: The init_nng function currently treats any nng_init result
other than NNG_OK/NNG_EBUSY as unreachable and doesn't validate
NngConfig.num_resolver_threads for negative values; update init_nng to validate
that cfg.num_resolver_threads.map(|n| n.get()) is strictly > 0 (return a config
validation error such as InitError::InvalidConfig if negative) before building
nng_init_params, and replace the final match on result from nng_init to handle
all other nng_err codes by returning a descriptive InitError variant (e.g.,
InitError::NngError(err) or InitError::Other(err)) instead of calling
unreachable! so unexpected errors (NNG_EINVAL, NNG_ENOMEM, etc.) are propagated
as errors.
♻️ Duplicate comments (1)
anng/src/init.rs (1)
124-137: Makedeinit_nngunsafe to reflect required preconditions.
The current safe signature is unsound because callers must guarantee all NNG resources are closed.🔒 Proposed fix
-/// # Safety Considerations +/// # Safety @@ -pub fn deinit_nng() { +pub unsafe fn deinit_nng() { // SAFETY: Caller is responsible for ensuring no NNG resources are in use. - unsafe { nng_sys::nng_fini() } + nng_sys::nng_fini() }
- impl Display for nng_err (std feature) - impl Error for nng_err (std feature) - Add as_str() method returning &'static str (no_std compatible) Addresses review comments: - nanomsg#37 (comment) - nanomsg#37 (comment)
Mark ThreadPoolConfig, NngConfig, and InitError as non_exhaustive to allow adding fields/variants in future minor releases without breaking semver compatibility. Add constructors: - ThreadPoolConfig::new(num, max) - NngConfig::new() Addresses review comment: nanomsg#37 (comment)
Add TODO comment referencing the tracking issue for CStr::display (rust-lang/rust#139984) to replace to_string_lossy() once stable.
…_err The original code used exact enum matching for NNG_ESYSERR and NNG_ETRANERR, which would never match actual system/transport errors because those are composite values (base flag OR'd with the specific error code). Fix by converting nng_err to u32 and using bitwise AND checks, matching the correct implementation already present in from_nz_u32.
| let mut socket = MaybeUninit::<nng_sys::nng_socket>::uninit(); | ||
| // SAFETY: socket pointer is valid for writing. | ||
| let errno = unsafe { nng_proto_open(socket.as_mut_ptr()) }; | ||
| match u32::try_from(errno).expect("errno is never negative") { |
There was a problem hiding this comment.
I think after we merge this PR, a follow-up should be to replace each of these with
match NonZeroU32::new(u32::try_from(errno).expect("errno is never negative")).map(AioError::from_nz_u32)(or that but through a helper so we don't have to repeat the whole mess each time)
That way we'll get nice matching semantics again.
I don't want to make it a blocker for landing this PR though, unless you specifically want to tackle it before merging — this review is already getting too long 😅
There was a problem hiding this comment.
Right. Think I changed those lines already a couple of times but always got back to this to make the diff smaller. Probably worth doing in a subsequent cleanup MR.
we expect to never get a nng_err::NNG_OK
Consolidate error handling infrastructure by moving error types from the anng crate to nng-sys, making them available at the lowest level of the crate hierarchy. Key changes: - Add ErrorCode enum to nng-sys for type-safe error code handling - Refactor error conversion and checking across the codebase - Simplify aio.rs by leveraging the new error infrastructure - Update all protocol implementations to use consistent error patterns This change improves error handling consistency and reduces code duplication by centralizing error definitions in the sys crate.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@anng/tests/init_default.rs`:
- Around line 3-7: The test init_with_default_config_succeeds should treat an
AlreadyInitialized error as a success to be resilient to parallel runs; update
the call to init_nng(Some(config)) inside the test to map or match the error
result so that if it returns an AlreadyInitialized error (the error variant
produced by init_nng), the test returns Ok(()) instead of failing, otherwise
propagate the original error; reference the init_with_default_config_succeeds
test, the init_nng function, NngConfig::default(), and the AlreadyInitialized
error variant when making the change.
In `@nng-sys/src/lib.rs`:
- Around line 211-279: The impl block for ErrorKind (methods from_nz_u32 and
from_nng_err) must be compiled only when the std feature is enabled; wrap the
entire impl ErrorKind { ... } with #[cfg(feature = "std")] (or gate the two
methods individually) so references to NonZeroU32 and std::io::Error are not
present in no_std builds, ensuring from_nz_u32, from_nng_err, and the
transport/system mapping logic are only available under the std feature.
- Around line 102-137: The codebase still references the removed NNG error
constants NNG_ENOARG and NNG_EAMBIGUOUS in the error mapping (e.g., the match
arms that reference nng_sys::NNG_ENOARG and nng_sys::NNG_EAMBIGUOUS); remove
those match arms and any branches that pattern-match on those symbols so the
error mapping aligns with the nng-sys ErrorCode enum (as defined by ErrorCode)
which omits values 28 and 29; ensure the match remains exhaustive (use a
fallback arm like `_` or the existing EINTERNAL/unknown handling used elsewhere)
and update any tests or code that expected those specific arms.
🧹 Nitpick comments (1)
anng/src/aio.rs (1)
376-421: Consider replacingunreachable!forEINTRwith a fallback.Lines 377-378 for
ETIMEDOUTandECANCELEDare correctly unreachable sincefrom_nng_errextracts these into dedicated variants. However, line 379's claim that "nng never exposes this publicly" forEINTRcould cause a panic if NNG's behavior changes or in edge cases.A defensive fallback would be safer:
Suggested defensive change
ErrorCode::ETIMEDOUT => unreachable!("always handled in outer variant"), ErrorCode::ECANCELED => unreachable!("always handled in outer variant"), - ErrorCode::EINTR => unreachable!("nng never exposes this publicly"), + ErrorCode::EINTR => io::Error::new(io::ErrorKind::Interrupted, err.to_string()),
Make the init module public (pub mod init) and only re-export init_nng in the root of the crate to avoid pollution. Users can access other init-related types via anng::init::* (e.g., anng::init::InitError, anng::init::NngConfig, anng::init::deinit_nng, etc.) Addresses review comment: nanomsg#37 (comment)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@anng/src/lib.rs`:
- Around line 326-349: The match fallback currently panics with unreachable! for
unmapped errno; instead, convert the raw errno into the library's ErrorKind and
return it as an AioError via the existing conversion path (e.g.
Err(AioError::Operation(ErrorKind::from(errno)))) so transport/system/other
errors are preserved; replace the final arm in the recv handling (the arm that
references errno and currently calls unreachable!) to build and return
Err(AioError::Operation(mapped_kind)) using ErrorKind conversion helpers (use
ErrorKind::from(errno) or the crate's equivalent) and include the original errno
in the created ErrorKind to retain context.
jonhoo
left a comment
There was a problem hiding this comment.
Nice, I think we're getting very close now!
Co-authored-by: Jon Gjengset <jon@thesquareplanet.com>
Add clearer explanation for why each init test file contains only one test: NNG configuration is static per process, and Rust runs each integration test file in a separate process.
Add comprehensive documentation for ErrorCode enum including: - Type-level documentation explaining the relationship to nng_err - Brief description for each error code variant - Reference to ErrorKind for unified error handling
- Remove AioError::from_nng_err() and from_nz_u32() helper methods - Handle NNG_ECANCELED and NNG_ETIMEDOUT explicitly in aio result check - Add ErrorKind::from_nz_u32() convenience method in nng-sys - Update protocol code to use AioError::Operation() directly with comments
| #[cfg(feature = "std")] | ||
| impl std::error::Error for ErrorKind {} | ||
|
|
||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
It's so sad that std::io::ErrorKind is only in std.
Migrate anng to NNG v2 API
Summary
This PR updates anng to support NNG v2.0, bringing typed error handling and the updated API surface.
Changes
API Updates
nng_errenum instead of rawu32error constantsnng_close→nng_socket_closenng_send_aio→nng_socket_sendnng_recv_aio→nng_socket_recvnng_strerrorsignature to takenng_errenumnng_init()(required by NNG 2.0)Dependency Updates
0.3.0to0.4.0-v2pre.1build.rsversion detection (no longer needed)#[cfg(nng_110)]feature gates — NNG 2.0 is now the baselineError Handling Improvements
nng_err::NNG_*variants with pattern guards for clearer matchingerrnaming throughout match patternsTest Updates
Breaking Changes
nng_errenum internallyOpen questions
AioError::Operationhold anng_sys::nng_err? Today the rawNonZeroU32error code value returned fromnngis stored in theAioError::Operationvariant. Most probably, morenngoperations will be updated to returnnng_errinstead ofc_intand storingnng_errwould feel more appropriate than the raw error code. The rationale behind storing the raw error for now is make this upgrade PR less invasive. Furthermore the NNG error docs states thatMany APIs are still using int, but the nng_err enumeration can be used instead. Another option would be to enroll all nng_err variants inAioError.nng_initfunctions to be called in advance to any other operation. For now I decided to ensure thatnng_initis called upon socket creation because I think there is no operation that can be initiated without holding a socket. Shall we expose a dedicated pub init? Fow now there is also no way to pass custom settings to init.Summary by CodeRabbit