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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jscheid
Copy link

@jscheid jscheid commented Nov 17, 2024

No description provided.

src/multi.rs Outdated
Comment on lines 985 to 989
/// Consumes the Easy2 handle, returning the underlying handler.
pub fn into_inner(self) -> H {
self.easy.into_inner()
}

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.

@@ -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.

Comment on lines +3726 to +3730
impl CURLPtr {
fn as_ptr(&self) -> *mut curl_sys::CURL {
self.0
}
}
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?

src/multi.rs Outdated
@@ -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?

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