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.

Sign up for GitHub

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

Add function into_inner to Easy and Easy2 (#586) #587

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+68 −29
Open

Add function into_inner to Easy and Easy2 (#586) #587

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e7c0b1
Add function into_inner to Easy and Easy2 (#586)
jscheid Nov 17, 2024
2434b62
fixup! Add function into_inner to Easy and Easy2 (#586)
jscheid Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 68 additions & 29 deletions src/easy/handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,12 @@ pub struct Easy2<H> {
inner: Box<Inner<H>>,
}

/// A thin wrapper around a low-level Easy handle that performs
/// cleanup when dropped.
struct CURLPtr(*mut curl_sys::CURL);
Copy link
Author

Choose a reason for hiding this comment

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

This is necessary because Inner can't be Drop anymore due to E0509, and because the following workarounds are unavailable:

  • We can't use std::mem::take because H isn't Default.
  • We don't want to use an Option because it adds overhead.


struct Inner<H> {
handle: *mut curl_sys::CURL,
handle: CURLPtr,
header_list: Option<List>,
resolve_list: Option<List>,
connect_to_list: Option<List>,
Expand Down Expand Up @@ -590,7 +594,7 @@ impl<H: Handler> Easy2<H> {
assert!(!handle.is_null());
let mut ret = Easy2 {
inner: Box::new(Inner {
handle,
handle: CURLPtr(handle),
header_list: None,
resolve_list: None,
connect_to_list: None,
Expand All @@ -611,7 +615,7 @@ impl<H: Handler> Easy2<H> {
/// cache, the dns cache, and cookies.
pub fn reset(&mut self) {
unsafe {
curl_sys::curl_easy_reset(self.inner.handle);
curl_sys::curl_easy_reset(self.inner.handle.as_ptr());
}
self.default_configure();
}
Expand Down Expand Up @@ -838,6 +842,11 @@ impl<H> Easy2<H> {
&mut self.inner.handler
}

/// Consumes the Easy handle, returning the underlying handler.
pub fn into_inner(self) -> H {
self.inner.handler
}

// =========================================================================
// Error options

Expand Down Expand Up @@ -3119,7 +3128,7 @@ impl<H> Easy2<H> {
unsafe {
let mut list = ptr::null_mut();
let rc = curl_sys::curl_easy_getinfo(
self.inner.handle,
self.inner.handle.as_ptr(),
curl_sys::CURLINFO_COOKIELIST,
&mut list,
);
Expand Down Expand Up @@ -3189,7 +3198,7 @@ impl<H> Easy2<H> {
/// call methods like `unpause_write` and `unpause_read` while a transfer is
/// in progress.
pub fn perform(&self) -> Result<(), Error> {
let ret = unsafe { self.cvt(curl_sys::curl_easy_perform(self.inner.handle)) };
let ret = unsafe { self.cvt(curl_sys::curl_easy_perform(self.inner.handle.as_ptr())) };
panic::propagate();
ret
}
Expand All @@ -3204,7 +3213,7 @@ impl<H> Easy2<H> {
/// called, an HTTP/2 PING frame is sent on the connection.
#[cfg(feature = "upkeep_7_62_0")]
pub fn upkeep(&self) -> Result<(), Error> {
let ret = unsafe { self.cvt(curl_sys::curl_easy_upkeep(self.inner.handle)) };
let ret = unsafe { self.cvt(curl_sys::curl_easy_upkeep(self.inner.handle.as_ptr())) };
panic::propagate();
return ret;
}
Expand All @@ -3225,7 +3234,10 @@ impl<H> Easy2<H> {
/// this function returns.
pub fn unpause_read(&self) -> Result<(), Error> {
unsafe {
let rc = curl_sys::curl_easy_pause(self.inner.handle, curl_sys::CURLPAUSE_RECV_CONT);
let rc = curl_sys::curl_easy_pause(
self.inner.handle.as_ptr(),
curl_sys::CURLPAUSE_RECV_CONT,
);
self.cvt(rc)
}
}
Expand All @@ -3246,7 +3258,10 @@ impl<H> Easy2<H> {
/// paused.
pub fn unpause_write(&self) -> Result<(), Error> {
unsafe {
let rc = curl_sys::curl_easy_pause(self.inner.handle, curl_sys::CURLPAUSE_SEND_CONT);
let rc = curl_sys::curl_easy_pause(
self.inner.handle.as_ptr(),
curl_sys::CURLPAUSE_SEND_CONT,
);
self.cvt(rc)
}
}
Expand All @@ -3258,7 +3273,7 @@ impl<H> Easy2<H> {
}
unsafe {
let p = curl_sys::curl_easy_escape(
self.inner.handle,
self.inner.handle.as_ptr(),
s.as_ptr() as *const _,
s.len() as c_int,
);
Expand Down Expand Up @@ -3291,7 +3306,7 @@ impl<H> Easy2<H> {
unsafe {
let mut len = 0;
let p = curl_sys::curl_easy_unescape(
self.inner.handle,
self.inner.handle.as_ptr(),
s.as_ptr() as *const _,
orig_len as c_int,
&mut len,
Expand Down Expand Up @@ -3340,7 +3355,7 @@ impl<H> Easy2<H> {
unsafe {
let mut n = 0;
let r = curl_sys::curl_easy_recv(
self.inner.handle,
self.inner.handle.as_ptr(),
data.as_mut_ptr() as *mut _,
data.len(),
&mut n,
Expand All @@ -3361,7 +3376,7 @@ impl<H> Easy2<H> {
unsafe {
let mut n = 0;
let rc = curl_sys::curl_easy_send(
self.inner.handle,
self.inner.handle.as_ptr(),
data.as_ptr() as *const _,
data.len(),
&mut n,
Expand All @@ -3373,7 +3388,7 @@ impl<H> Easy2<H> {

/// Get a pointer to the raw underlying CURL handle.
pub fn raw(&self) -> *mut curl_sys::CURL {
self.inner.handle
self.inner.handle.as_ptr()
}

#[cfg(unix)]
Expand All @@ -3392,15 +3407,27 @@ impl<H> Easy2<H> {
}

fn setopt_long(&mut self, opt: curl_sys::CURLoption, val: c_long) -> Result<(), Error> {
unsafe { self.cvt(curl_sys::curl_easy_setopt(self.inner.handle, opt, val)) }
unsafe {
self.cvt(curl_sys::curl_easy_setopt(
self.inner.handle.as_ptr(),
opt,
val,
))
}
}

fn setopt_str(&mut self, opt: curl_sys::CURLoption, val: &CStr) -> Result<(), Error> {
self.setopt_ptr(opt, val.as_ptr())
}

fn setopt_ptr(&self, opt: curl_sys::CURLoption, val: *const c_char) -> Result<(), Error> {
unsafe { self.cvt(curl_sys::curl_easy_setopt(self.inner.handle, opt, val)) }
unsafe {
self.cvt(curl_sys::curl_easy_setopt(
self.inner.handle.as_ptr(),
opt,
val,
))
}
}

fn setopt_off_t(
Expand All @@ -3409,7 +3436,7 @@ impl<H> Easy2<H> {
val: curl_sys::curl_off_t,
) -> Result<(), Error> {
unsafe {
let rc = curl_sys::curl_easy_setopt(self.inner.handle, opt, val);
let rc = curl_sys::curl_easy_setopt(self.inner.handle.as_ptr(), opt, val);
self.cvt(rc)
}
}
Expand All @@ -3421,7 +3448,13 @@ impl<H> Easy2<H> {
flags: curl_sys::CURL_BLOB_COPY,
};
let blob_ptr = &blob as *const curl_sys::curl_blob;
unsafe { self.cvt(curl_sys::curl_easy_setopt(self.inner.handle, opt, blob_ptr)) }
unsafe {
self.cvt(curl_sys::curl_easy_setopt(
self.inner.handle.as_ptr(),
opt,
blob_ptr,
))
}
}

fn getopt_bytes(&self, opt: curl_sys::CURLINFO) -> Result<Option<&[u8]>, Error> {
Expand All @@ -3438,7 +3471,7 @@ impl<H> Easy2<H> {
fn getopt_ptr(&self, opt: curl_sys::CURLINFO) -> Result<*const c_char, Error> {
unsafe {
let mut p = ptr::null();
let rc = curl_sys::curl_easy_getinfo(self.inner.handle, opt, &mut p);
let rc = curl_sys::curl_easy_getinfo(self.inner.handle.as_ptr(), opt, &mut p);
self.cvt(rc)?;
Ok(p)
}
Expand All @@ -3458,7 +3491,7 @@ impl<H> Easy2<H> {
fn getopt_long(&self, opt: curl_sys::CURLINFO) -> Result<c_long, Error> {
unsafe {
let mut p = 0;
let rc = curl_sys::curl_easy_getinfo(self.inner.handle, opt, &mut p);
let rc = curl_sys::curl_easy_getinfo(self.inner.handle.as_ptr(), opt, &mut p);
self.cvt(rc)?;
Ok(p)
}
Expand All @@ -3467,7 +3500,7 @@ impl<H> Easy2<H> {
fn getopt_double(&self, opt: curl_sys::CURLINFO) -> Result<c_double, Error> {
unsafe {
let mut p = 0 as c_double;
let rc = curl_sys::curl_easy_getinfo(self.inner.handle, opt, &mut p);
let rc = curl_sys::curl_easy_getinfo(self.inner.handle.as_ptr(), opt, &mut p);
self.cvt(rc)?;
Ok(p)
}
Expand Down Expand Up @@ -3513,20 +3546,12 @@ impl<H> Easy2<H> {
impl<H: fmt::Debug> fmt::Debug for Easy2<H> {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.debug_struct("Easy")
.field("handle", &self.inner.handle)
.field("handle", &self.inner.handle.as_ptr())
.field("handler", &self.inner.handler)
.finish()
}
}

impl<H> Drop for Easy2<H> {
fn drop(&mut self) {
unsafe {
curl_sys::curl_easy_cleanup(self.inner.handle);
}
}
}

extern "C" fn header_cb<H: Handler>(
buffer: *mut c_char,
size: size_t,
Expand Down Expand Up @@ -3698,6 +3723,20 @@ fn double_seconds_to_duration_sub_second2() {
assert_eq!(dur.subsec_nanos(), 500_000_000);
}

impl CURLPtr {
fn as_ptr(&self) -> *mut curl_sys::CURL {
self.0
}
}
Comment on lines +3726 to +3730
Copy link
Author

Choose a reason for hiding this comment

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

Since the struct is package-private, perhaps this is too much ceremony and we should just reach into it with .0 everywhere. I don't think there's a perf difference, mostly a matter of taste. Happy to remove this.

Copy link
Author

Choose a reason for hiding this comment

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

Or we could impl Deref - maybe that would be nicer?


impl Drop for CURLPtr {
fn drop(&mut self) {
unsafe {
curl_sys::curl_easy_cleanup(self.0);
}
}
}

impl Auth {
/// Creates a new set of authentications with no members.
///
Expand Down
5 changes: 5 additions & 0 deletions src/multi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,11 @@ impl<H> Easy2Handle<H> {
self.easy.get_mut()
}

/// Consumes the Easy2 handle, returning the underlying handler.
pub fn into_inner(self) -> H {
self.easy.into_inner()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this cause drops to be performed in the incorrect order? It seems like self.easy.inner.handle will be dropped first, triggering curl_easy_cleanup, and then at the end of the function, self.guard will be dropped, triggering curl_multi_remove_handle. Whereas they need to happen in the opposite order.

To fix this, I believe calling self.guard.detach()? first before extracting the inner value should be sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

I'm trying to build a failing test case for this but without much luck:

Adding the Easy to the Multi moves ownership of the Easy into the Multi (since Multi::add takes a mut Easy). This means that into_inner can't be called by client code anymore after adding, since into_inner takes self and therefore requires ownership.

To regain ownership, the client code first has to call Multi::remove, meaning that the detach guard never comes into play.

I could be missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Multi::add takes ownership of Easy2, but then passes ownership to the Easy2Handle, which then owns the Easy2. Example:

use curl::{easy::{Easy2, Handler}, multi::Multi};

struct DummyHandler;

impl Handler for DummyHandler {}

fn main() {
    let easy = Easy2::new(DummyHandler);
    let multi = Multi::new();
    let handle = multi.add2(easy).unwrap();
    let handler = handle.into_inner();
}

And definition of Easy2Handle::into_inner annotated:

    pub fn into_inner(self) -> H {
        // took ownership of `self`
        self.easy.into_inner() // takes ownership of `self.easy`, and then drops it
        // `self` is dropped here, including `self.guard`
    }

And Easy2::into_inner:

    pub fn into_inner(self) -> H {
        // took ownership of `self`
        self.inner.handler
        // `self` is dropped here, including `self.inner.handle`
    }

If you add a println! where curl_multi_remove_handle and curl_easy_cleanup are called, you'll see in my example that curl_easy_cleanup is called first, and then curl_multi_remove_handle.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry - I didn't mean to add into_inner to Easy2Handle in the first place. I've pushed a fixup commit to remove it, does it look better now?

}

Copy link
Author

Choose a reason for hiding this comment

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

Maybe this should return an Easy instead? It wouldn't be symmetric with new however, and not sure if there's much value in it.

/// Same as `EasyHandle::set_token`
pub fn set_token(&mut self, token: usize) -> Result<(), Error> {
unsafe {
Expand Down