Skip to content

add ngx::core::resolver, which provides async name resolution #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pchickey
Copy link
Contributor

@pchickey pchickey commented Aug 18, 2025

Based on #200

Added behind its own feature flag, because it incurs two extra deps (thiserror and futures-channel) on top of the async feature set.

This PR is an upstreaming of nginx-acme::net::resolver, see: https://github.com/nginx/nginx-acme/blob/main/src/net/resolver.rs

There are no examples or tests for this module at this time.

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

@bavshin-f5
Copy link
Member

  1. The implementation is async-only, so it seems to fit better under src/async_.
  2. thiserror is an acceptable dependency, but in this case it could be replaced with a trivial impl core::fmt::Display + impl core::error::Error.
  3. I'd actually prefer an implementation without futures-channel. There are two heap allocations we can avoid by making this a manual future with proper pinning. And then there's just one extra step to make the code no_std.
  4. I'd like to keep the original names for NGX_RESOLVER_ status codes. RCODEs like ServFail or NXDomain come directly from the DNS specification and renaming them just makes things extra confusing.

For the record, I already raised 3-4 during the review for acme, and we decided to defer it until the ngx-rust PR.

@pchickey pchickey force-pushed the pch/upstream_resolver branch from 08fd958 to 4becb06 Compare August 19, 2025 00:11
@pchickey
Copy link
Contributor Author

pchickey commented Aug 19, 2025

Thanks, implemented 1, 2, and 4.

I'd actually prefer an implementation without futures-channel. There are two heap allocations we can avoid by making this a manual future with proper pinning. And then there's just one extra step to make the code no_std.

Its not obvious to me where these two heap allocations are that we can avoid with proper pinning, and I don't see where it was discussed in the closed PRs. If we're going to tear out the channel, the most obvious improvement to me would be to handle the receiver.await future's cancellation by correctly calling ngx_resolve_name_done. I can't convince myself the current impl Drop for ResCtx is ever reachable when ctx is some.

@bavshin-f5
Copy link
Member

Its not obvious to me where these two heap allocations are that we can avoid with proper pinning, and I don't see where it was discussed in the closed PRs.

futures_channel::oneshot::{Sender,Receiver} share an Arc<Inner>. That's one allocation and several unnecessary atomic operations.
Box<ResCtx> is another allocation that we do for the sole reason of having a callback context with fixed address. A pinned impl Future is guaranteed to have a stable address and can be used as a data for ngx_resolver_ctx_t instead.

And one more heap allocation is in the resulting Vec which could be using Pool instead.

If we're going to tear out the channel, the most obvious improvement to me would be to handle the receiver.await future's cancellation by correctly calling ngx_resolve_name_done. I can't convince myself the current impl Drop for ResCtx is ever reachable when ctx is some.

Oh. Right, we consume the Box and it is no longer responsible for dropping rctx. That is a potential leak.

enforced by workspace level lints, now specified in each Cargo.toml,
except nginx-sys, because bindgen can't be set to Edition2024 until MSRV
hits 1.85

this change set is created automatically by `cargo fix --edition --all`,
then with manual fixes afterwards:

* Back out any use of unsafe(no_mangle) for plain no_mangle, since 1.81
  wont accept it
* remove uses of expr_2021 in macros, which is not available in the
  MSRV.
* Manual fix to ngx_container_of! macro for safety rules in 2024,
  these weren't migrated automatically by `cargo fix`
* Manual fixes to several other macros that created an &mut Request in an
  expression, Rust 2024 is stricter about taking &mut of temporaries, so
  instead the request is let-bound first.
@pchickey pchickey force-pushed the pch/upstream_resolver branch from 4becb06 to 0205d33 Compare August 21, 2025 21:54
Added behind the async feature, and adds the futures-channel dep to
to the deps enabled by async.

This PR is an upstreaming of nginx-acme::net::resolver, see: https://github.com/nginx/nginx-acme/blob/main/src/net/resolver.rs

Co-authored-by: Evgeny Shirykalov <[email protected]>
@pchickey pchickey force-pushed the pch/upstream_resolver branch from 0205d33 to 6ce390b Compare August 21, 2025 21:59
@pchickey
Copy link
Contributor Author

Feedback addressed, I rewrote it as a Future by hand. It still requires one Box because Pin::new requires the inner to have Deref. Tests exist in another repo where @bavshin-f5 can see them. I can port tests to this repo once #198 lands as a basis, but that PR is stuck on an unrelated issue.

I will squash this down once feedback is complete.

@pchickey pchickey requested a review from bavshin-f5 August 22, 2025 00:33
@pchickey pchickey force-pushed the pch/upstream_resolver branch from b9bef62 to 75add6c Compare August 22, 2025 23:06
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