Skip to content

Conversation

ensh63
Copy link
Contributor

@ensh63 ensh63 commented Sep 11, 2025

Proposed changes

foreign_type::ForeignTypeRef trait is used to define Request object. While generally the use of this object almost unchanged, reference trait allows to avoid unnecessary copying of original ngx_http_request_t data.

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 checked that all unit tests pass after adding my changes
  • 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

@ensh63 ensh63 requested a review from Copilot September 11, 2025 18:26
Copy link

@Copilot 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 refactors the Request struct to use the foreign_types::ForeignTypeRef trait instead of direct wrapper implementation. This change allows the Request object to avoid unnecessary copying of original ngx_http_request_t data by using a reference-based approach.

Key changes:

  • Replace direct struct wrapping with ForeignTypeRef trait implementation
  • Update all macro call sites to use ForeignTypeRef::from_ptr_mut() instead of custom conversion
  • Add foreign-types dependency to enable the new pattern

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/http/request.rs Refactored Request struct to implement ForeignTypeRef trait and updated all field access to use helper methods
src/http/upstream.rs Updated macro to use ForeignTypeRef::from_ptr_mut() for request conversion
src/core/mod.rs Added re-export of ForeignTypeRef trait
examples/upstream.rs Updated example to use new ForeignTypeRef pattern and added import
examples/async.rs Updated async example to use ForeignTypeRef conversion methods
Cargo.toml Added foreign-types dependency

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

@ensh63 ensh63 requested review from bavshin-f5 and xeioex September 11, 2025 18:34
Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

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

I agree that Request(ngx_http_request_t) was the wrong representation. I was going to propose that we make it Request(NonNull<ngx_http_request_t>) with a constructor fn from_http_request(NonNull<ngx_http_request_t>) -> Self and not use any &mut self methods, because nginx does not consider any *mut ngx_http_request_t to be unique, and the uniqueness property of Request does not make its use any more or less unsafe.

@ensh63 ensh63 force-pushed the shirykalov/request branch 3 times, most recently from f023297 to 82b683d Compare September 12, 2025 00:25
Copy link
Contributor

@xeioex xeioex left a comment

Choose a reason for hiding this comment

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

LGTM.

@ensh63 ensh63 self-assigned this Sep 12, 2025
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.

3 participants