-
Notifications
You must be signed in to change notification settings - Fork 81
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
Conversation
For the record, I already raised 3-4 during the review for acme, and we decided to defer it until the ngx-rust PR. |
08fd958 to
4becb06
Compare
|
Thanks, implemented 1, 2, and 4.
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 |
And one more heap allocation is in the resulting
Oh. Right, we consume the |
0205d33 to
6ce390b
Compare
|
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. |
b9bef62 to
75add6c
Compare
75add6c to
95d8b4f
Compare
|
No longer based on #200 |
363b648 to
44c3e67
Compare
|
We're now satisfied with this implementation, and I have squashed it down to a single commit based on the latest main. |
There was a problem hiding this 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.
There was a problem hiding this 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>
44c3e67 to
f5f1077
Compare
|
Added typo fix found by Copilot. |
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.