Skip to content
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

WIP: Issues 30 loadbalancer #35

Closed
wants to merge 5 commits into from

Conversation

f5yacobucci
Copy link
Contributor

WIP checkpoint for load balancer examples.

#30

src/http/request.rs Outdated Show resolved Hide resolved
Comment on lines 184 to 195
let upstream_ptr = request.upstream();

(*hcpd).conf = Some(hccf);
(*hcpd).upstream = upstream_ptr;
(*hcpd).data = (*upstream_ptr).peer.data;
(*hcpd).client_connection = request.connection();
(*hcpd).original_get_peer = (*upstream_ptr).peer.get;
(*hcpd).original_free_peer = (*upstream_ptr).peer.free;

(*upstream_ptr).peer.data = hcpd as *mut c_void;
(*upstream_ptr).peer.get = Some(ngx_http_upstream_get_custom_peer);
(*upstream_ptr).peer.free = Some(ngx_http_upstream_free_custom_peer);
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: potentially it is a good use case to "wrap upstream" and see what we can do with a new "wrapped" upstream rust type... not insisting on the current PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's preferable, to wrap it. I've alluded a little to that in the FIXME's. I'm still thinking through how that should look - it's a little complicated to me ATM.

There are a significant number of structures and functions that accept an ngx_http_upstream_t as an argument.

  • upstream peers
  • upstream nexts
  • upstream headers
  • upstream locals
  • upstream servers
  • upstream state

Copy link
Contributor

@ivanitskiy ivanitskiy Jun 26, 2023

Choose a reason for hiding this comment

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

true, then similar to Bufferalnog these lines:

pub struct NgxHttpUpstream(*mut ngx_http_upstream_t);

impl NgxHttpUpstream {

   /// Create a [`NgxHttpUpstream`] from an [`ngx_http_request_t`].
    pub unsafe fn from_ngx_http_upstream<'a>(r: *mut ngx_http_upstream_t) -> &'a mut NgxHttpUpstream {
        &mut *r.cast::<NgxHttpUpstream>()
    }
    
    /// Returns the underlying `ngx_http_upstream_t ` pointer as a raw pointer.
    pub fn as_ngx(&self) -> *const ngx_http_upstream_t {
        self.0
    }

    /// Returns a mutable reference to the underlying `ngx_http_upstream_t ` pointer.
    pub fn as_ngx_mut(&mut self) -> *mut ngx_http_upstream_t {
        self.0
    }
}

we simply can just have only these 3 methods and in the module example use


let req = unsafe { &mut http::Request::from_ngx_http_request(r) };
// instantiate wrapped upstream..
let u = unsafe{ req.from_ngx_http_upstream(r.upstream_ptr)) };

// and somewhere in the code block where we have a reference to NgxHttpUpstream as `u` we can get a raw pointer  now:
u.as_ngx_mut()

what do you think? and now the biggest question is it ergonomic? your opinion values here as a module developer and is it readable, etc?

additionally, we can try to think about implementing From trait. so we can implicitly pass upstream...
i hope it is more clear what the high-level direction I'm thinking about is. Please challenge me here as I may be wrong.

Copy link
Contributor Author

@f5yacobucci f5yacobucci Jun 27, 2023

Choose a reason for hiding this comment

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

I was leaning towards From trait, but need to experiment a little.

I'm hoping a wrapped upstream can help with the get and free peer functions. If that requires another peer wrapper I need to take that into consideration, but I'd like to hide the Rust -> C func-ptr transmute casts if possible, a macro could possibly hide the boiler plate too perhaps. On the other hand, maybe it just requires Some() around the func pointers. Once again, need to experiment a little more.

Update: get and free are already Options. the transmute was unecessary. still looking at an Upstream wrapper - will follow.

examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
examples/upstream.rs Outdated Show resolved Hide resolved
src/http/request.rs Outdated Show resolved Hide resolved
@f5yacobucci
Copy link
Contributor Author

Adding the upstream method should increment the MINOR version.

/// The caller has provided a value `ngx_http_upstream_srv_conf_t. If the `us` argument is null, a
/// None Option is returned; however, if the `us` internal fields are invalid or the module index
/// is out of bounds failures may still occur.
pub unsafe fn ngx_http_conf_upstream_srv_conf_mutable<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

based on this: https://rust-lang.github.io/api-guidelines/naming.html then it may be renamed:
ngx_http_conf_upstream_srv_conf_as_mut

/// The caller has provided a value `ngx_http_upstream_srv_conf_t. If the `us` argument is null, a
/// None Option is returned; however, if the `us` internal fields are invalid or the module index
/// is out of bounds failures may still occur.
pub unsafe fn ngx_http_conf_upstream_srv_conf_immutable<T>(
Copy link
Contributor

Choose a reason for hiding this comment

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

seems to me based on this https://doc.rust-lang.org/std/primitive.pointer.html we can name as ngx_http_conf_upstream_srv_conf_as_ptr or

some ref: https://doc.rust-lang.org/src/core/ptr/const_ptr.rs.html#1629

@f5yacobucci
Copy link
Contributor Author

Closing and re-opening with split commits and working version.

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