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 and others added 4 commits August 27, 2025 12:33
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 <e.shirykalov@f5.com>
@pchickey pchickey force-pushed the pch/upstream_resolver branch from 75add6c to 95d8b4f Compare August 27, 2025 19:34
@pchickey
Copy link
Contributor Author

No longer based on #200

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