Skip to content

Conversation

@pchickey
Copy link
Contributor

@pchickey pchickey commented Aug 18, 2025

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.

@pchickey pchickey force-pushed the pch/upstream_resolver branch 2 times, most recently 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
Copy link
Contributor Author

No longer based on #200

@pchickey pchickey requested a review from bavshin-f5 September 9, 2025 22:30
@pchickey pchickey force-pushed the pch/upstream_resolver branch from 363b648 to 44c3e67 Compare September 12, 2025 20:23
@pchickey
Copy link
Contributor Author

We're now satisfied with this implementation, and I have squashed it down to a single commit based on the latest main.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds asynchronous DNS resolution capabilities to the nginx crate by introducing a new ngx::core::resolver module. The module provides a Rust async wrapper around nginx's native resolver functionality, enabling async name and service resolution.

Key changes:

  • Added async DNS resolver with proper error handling and Future implementation
  • Exposed Pool's underlying ngx_pool_t pointer for FFI interoperability
  • Integrated resolver module into the async module structure

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/core/pool.rs Added as_ptr() method to expose underlying ngx_pool_t pointer
src/async_/resolver.rs New module implementing async DNS resolution with comprehensive error handling
src/async_/mod.rs Added resolver module to async module exports

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@bavshin-f5 bavshin-f5 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

Added behind the async feature.

Additionally, add an `as_ptr` method to ngx::core::Pool.

This was originally an upstreaming of nginx-acme::net::resolver, but was
rewritten during this upstreaming process see:
https://github.com/nginx/nginx-acme/blob/main/src/net/resolver.rs

There are no tests for this functionality in the ngx-rust repo at this
time. This PR has been tested by manually using a test suite in a
private nginx repo. Tests will be added in this repo at some point in
the future.

Co-authored-by: Evgeny Shirykalov <e.shirykalov@f5.com>
Co-authored-by: Aleksei Bavshin <a.bavshin@f5.com>
@pchickey pchickey force-pushed the pch/upstream_resolver branch from 44c3e67 to f5f1077 Compare September 12, 2025 20:58
@pchickey
Copy link
Contributor Author

Added typo fix found by Copilot.

@bavshin-f5 bavshin-f5 merged commit 783f68f into nginx:main Sep 12, 2025
15 checks passed
@pchickey pchickey deleted the pch/upstream_resolver branch September 12, 2025 21:02
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