Add support for curl_easy_upkeep

[cmeister2]

Add feature-gated support for curl_easy_upkeep, introduced in 7.62.0

---

Happy to discuss feature naming, etc! Would be nice to have a model for adding in features that have been introduced within the last 9 years.

[alexcrichton]

Thanks for this! I wonder, though, would it be possible to do some quick-and-dirty version detection of the libcurl found at build time? That way we could change this function to always be available and simply return an error unconditionally if the linked version doesn't have support.

[sagebind]

Even that won't always work though, because if we are linking to libcurl dynamically then the build machine might have a newer version of libcurl than the user's machine. Depending on the system you'll probably get undefined symbol errors when the OS tries to run your program and the only way to opt-out would be to downgrade libcurl to an old enough version on your build machine.

[cmeister2]

I guess for dynamic curl one option might be to use something like libloading to dynamically load the library, and try and get the relevant function pointer, failing the call if it's not found.

For static curl it could be determined at build time.

On Thu, 25 Feb 2021, 18:15 Stephen M. Coakley, notifications@github.com wrote:

Even that won't always work though, because if we are linking to libcurl dynamically then the build machine might have a newer version of libcurl than the user's machine. Depending on the system you'll probably get undefined symbol errors when the OS tries to run your program and the only way to opt-out would be to downgrade libcurl to an old enough version on your build machine.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alexcrichton/curl-rust/pull/378#issuecomment-786101485, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPA36MMH2BPKP42NPUIAR3TA2HTRANCNFSM4YGXDDBA .

[cmeister2]

I also realised I missed a few files off - let me push those before pushing anything else.

[cmeister2]

The code I've written for dynamic loading is this:

    /// Some protocols have "connection upkeep" mechanisms. These mechanisms
    /// usually send some traffic on existing connections in order to keep them
    /// alive; this can prevent connections from being closed due to overzealous
    /// firewalls, for example.
    ///
    /// Currently the only protocol with a connection upkeep mechanism is
    /// HTTP/2: when the connection upkeep interval is exceeded and upkeep() is
    /// called, an HTTP/2 PING frame is sent on the connection.
    #[cfg(not(feature = "static-curl"))]
    pub fn upkeep(&self) -> Result<(), Error> {
        // Derive the curl binary name
        let libname = libloading::library_filename("curl");

        let ret = unsafe {
            let lib = libloading::Library::new(&libname).map_err(|_| {
                let mut e = Error::new(curl_sys::CURLE_FAILED_INIT);
                e.set_extra(format!("Failed to load library: {:?}", libname));
                e
            })?;

            let func = lib
                .get::<unsafe extern "C" fn(*mut curl_sys::CURL) -> curl_sys::CURLcode>(
                    b"curl_easy_upkeep",
                )
                .map_err(|_| {
                    let mut e = Error::new(curl_sys::CURLE_FAILED_INIT);
                    e.set_extra("Failed to find function: curl_easy_upkeep".to_string());
                    e
                })?;

            self.cvt(func(self.inner.handle))
        };

        panic::propagate();
        return ret;
    }

The test_upkeep test fails with

---- test_upkeep stdout ----
thread 'test_upkeep' panicked at 'handle.upkeep() failed with Error { description: "Failed initialization", code: 2, extra: Some("Failed to find function: curl_easy_upkeep") }', tests/easy.rs:849:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

with dynamic libcurl 7.60.0, and passes with dynamic libcurl 7.76.0. "static-curl" curl works as well regardless.

@alexcrichton - thoughts on the above? If you want me to explore using libc::dlsym instead of libloading I can (to reduce the number of dependencies) but afaict libloading makes it ergonomic to use those functions.

[sagebind]

Interesting approach, I'll have to think about this. Some initial thoughts:

• We should probably return some sort of "your curl does not support this method" error if the symbol cannot be found.
• We should probably avoid loading libraries on every method call and instead do it just once lazily as needed. Otherwise it could be expensive.
• There's a potential safety risk here if the libcurl that dynamic loading finds gets a different binary than the one the application is linked to, which could happen if the user has more than one libcurl installed. We'd be calling methods from one libcurl version on a handle created by a different libcurl version.

[cmeister2]

Completely agree on your last point - I'm not sure how to reconcile it. I did a little research into dlsym and dlopen but couldn't immediately see an answer. If I have time I'll dig a little deeper.

On Thu, 25 Feb 2021, 21:42 Stephen M. Coakley, notifications@github.com wrote:

Interesting approach, I'll have to think about this. Some initial thoughts:

• We should probably return some sort of "your curl does not support this method" error if the symbol cannot be found.
• We should probably avoid loading libraries on every method call and instead do it just once lazily as needed. Otherwise it could be expensive.
• There's a potential safety risk here if the libcurl that dynamic loading finds gets a different binary than the one the application is linked to, which could happen if the user has more than one libcurl installed. We'd be calling methods from one libcurl version on a handle created by a different libcurl version

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alexcrichton/curl-rust/pull/378#issuecomment-786247236, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPA36KDUAESYLPBTZY2OVTTA274JANCNFSM4YGXDDBA .

[cmeister2]

Looking at the man page for dlopen, it seems that if you use a NULL pointer as your argument to dlopen, it will allow you to search the main program plus any loaded shared libraries for a given symbol name, which seems perfect. I'll see how that maps into libloading (if at all) and make a patch tomorrow.

On Thu, 25 Feb 2021, 23:03 Max Dymond, cmeister2@gmail.com wrote:

Completely agree on your last point - I'm not sure how to reconcile it. I did a little research into dlsym and dlopen but couldn't immediately see an answer. If I have time I'll dig a little deeper.

On Thu, 25 Feb 2021, 21:42 Stephen M. Coakley, notifications@github.com wrote:

Interesting approach, I'll have to think about this. Some initial thoughts:

• We should probably return some sort of "your curl does not support this method" error if the symbol cannot be found.
• We should probably avoid loading libraries on every method call and instead do it just once lazily as needed. Otherwise it could be expensive.
• There's a potential safety risk here if the libcurl that dynamic loading finds gets a different binary than the one the application is linked to, which could happen if the user has more than one libcurl installed. We'd be calling methods from one libcurl version on a handle created by a different libcurl version

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/alexcrichton/curl-rust/pull/378#issuecomment-786247236, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPA36KDUAESYLPBTZY2OVTTA274JANCNFSM4YGXDDBA .

[cmeister2]

Seems as if you can do this with libloading, though for some reason this is not exposed on the top level Library in libloading.

    #[cfg(not(feature = "static-curl"))]
    pub fn upkeep(&self) -> Result<(), Error> {
        let ret = unsafe {
            let lib = libloading::os::unix::Library::this();
            let func = lib
                .get::<unsafe extern "C" fn(*mut curl_sys::CURL) -> curl_sys::CURLcode>(
                    b"curl_easy_upkeep",
                )
                .map_err(|_| {
                    let mut e = Error::new(curl_sys::CURLE_FAILED_INIT);
                    e.set_extra("Failed to find function: curl_easy_upkeep".to_string());
                    e
                })?;

            self.cvt(func(self.inner.handle))
        };

        panic::propagate();
        return ret;
    }

Whether that's easier to use I don't know! There does appear to be similar support in the Windows Library as well for this but am unsure it works in the same way.

[alexcrichton]

That's a good point about build-time vs runtime, but I think that it may be a concern best handled by applications themselves rather than this crate. For example if we unconditionally define the method here (not even do version detection), it shouldn't cause problems if you don't actually even call the method in your application (the function is GC'd away and the symbol is never referenced).

In that sense I don't think that adding this symbol is a detriment to existing users. I'd prefer to avoid detection if possible because it tends to be pretty heavyweight and not always the most portable.

Perhaps this could do build-time detection, and then eschew runtime detection to the caller? (e.g. applications would be responsible for providing the right knobs to doing this detection at runtime if necessary).

[cmeister2]

I haven't seen a good way of fitting build-time detection in with the existing build.rs code; presumably you'd want to do something like:

• in curl-sys, try and build against "the library" to detect the function, ala AC_CHECK_LIB.
  • not trivial because pkgconfig just outputs cargo build lines to stdout, rather than in an inspectable way.
• if function is detected, set cfg variable with cargo:rustc-cfg=KEY[="VALUE"]
• expose this up the chain somehow so that curl knows that curl-sys was compiled with a certain feature enabled.

There's a lot of steps there that I'm handwaving away because I'm not really sure how to achieve them.

[alexcrichton]

If you'd prefer to keep this as a Cargo feature that seems fine too. My experience is that this makes it pretty non-composable and this is pretty unlikely to ever get used as a result. By putting some extra work in it's possible to make it much more ergonomic to use and possible for consumers to use in more circumstances.

[cmeister2]

In the short term I think I'd prefer "merged and using a feature" over "rewriting the build script system and not yet merged" :smile: After that I'd be happy to throw some thinking and prototyping power at doing feature detection - surely this must be solved in other -sys libraries, so I can do a bit of investigation there.

[alexcrichton]

Ok sounds fine by me.