Get rid of curl_easy_reset (#192)

d297b12 introduced eager curl handle initialization and its further reusage to leverage from connection caching.

It's definitely a movement into right direction but current code has a problem: it removes all the presets from the handle with `curl_easy_reset` (though it doesn't reset caches).

There's currently a bug that resets progress callback been set in `session_alloc`.

I propose to move the initialization of option defaults and all the options which are not request-specific to `session_alloc` and apply it to another handle called `base_handle`.

Then, each request builds a duplicate of that `base_handle` and sets request-specific options on it. Connection cache is being shared using curl shared objects.

Further, I'd like to refactor `set_options_from_request` function to avoid handle duplication at all in cases when no options are overriden on request level. It'd require to rewrite all `Session` option accessors to modify on `base_handle` directly so all the session-level options will already be applied and less work needs to be done on each request.

Handle duplication: https://curl.se/libcurl/c/curl_easy_duphandle.html
Shared objects: https://curl.se/libcurl/c/curl_share_init.html

README.md

@@ -89,7 +89,7 @@ Sharing Session objects between requests will also allow you to benefit from per
 
 ## Persistent connections
 
-Patron follows the libCURL guidelines on [connection reuse.](https://ec.haxx.se/libcurl-connectionreuse.html) If you create the Session
+Patron follows the libCURL guidelines on [connection reuse.](https://everything.curl.dev/libcurl/connectionreuse.html) If you create the Session
 object once and use it for multiple requests, the same libCURL handle is going to be used across these requests and if requests go to
 the same hostname/port/protocol the connection should get reused.
 

ext/patron/extconf.rb

@@ -19,4 +19,8 @@
   $CFLAGS << ' -pedantic -Wall'
 end
 
+if CONFIG['CC'] =~ /clang/
+  $CFLAGS << ' -pedantic -Wall -Wno-void-pointer-to-enum-cast'
+end
+
 create_makefile 'patron/session_ext'

ext/patron/session_ext.c

@@ -27,6 +27,8 @@ static VALUE eAborted = Qnil;
 
 struct patron_curl_state {
   CURL* handle;
+  CURL* base_handle;
+  CURLSH* share;
   char* upload_buf;
   FILE* download_file;
   FILE* debug_file;
@@ -75,7 +77,7 @@ static size_t file_write_handler(void* stream, size_t size, size_t nmemb, FILE*
 static int call_user_rb_progress_blk(void* vd_curl_state) {
   struct patron_curl_state* state = (struct patron_curl_state*)vd_curl_state;
   // Invoke the block with the array
-  VALUE blk_result = rb_funcall(state->user_progress_blk,
+  rb_funcall(state->user_progress_blk,
     rb_intern("call"), 4,
     LONG2NUM(state->dltotal),
     LONG2NUM(state->dlnow),
@@ -175,20 +177,18 @@ static void session_close_debug_file(struct patron_curl_state *curl) {
 }
 
 /* Cleans up the patron_curl_state data when the Session object is garbage collected. */
-void session_free(struct patron_curl_state *curl) {
-  if (curl->handle) {
-    curl_easy_cleanup(curl->handle);
-    curl->handle = NULL;
-  }
+void session_free(struct patron_curl_state *state) {
+  curl_easy_cleanup(state->base_handle);
+  curl_share_cleanup(state->share);
 
-  session_close_debug_file(curl);
+  session_close_debug_file(state);
 
-  membuffer_destroy( &curl->header_buffer );
-  membuffer_destroy( &curl->body_buffer );
+  membuffer_destroy(&state->header_buffer);
+  membuffer_destroy(&state->body_buffer);
 
-  cs_list_remove(curl);
+  cs_list_remove(state);
 
-  free(curl);
+  free(state);
 }
 
 /* Allocates patron_curl_state data needed for a new Session object. */
@@ -206,16 +206,34 @@ VALUE session_alloc(VALUE klass) {
     reuse the TCP connection and can speed things up if the same resource - like a backend service -
     gets accessed over and over with requests.
   */
-  state->handle = curl_easy_init();
-  curl_easy_setopt(state->handle, CURLOPT_NOSIGNAL, 1);
-  curl_easy_setopt(state->handle, CURLOPT_NOPROGRESS, 0);
-  #if LIBCURL_VERSION_NUM >= 0x072000
-    /* this is libCURLv7.32.0 or later, supports CURLOPT_XFERINFOFUNCTION */
-    curl_easy_setopt(state->handle, CURLOPT_XFERINFOFUNCTION, &session_progress_handler);
-  #else
-    curl_easy_setopt(state->handle, CURLOPT_PROGRESSFUNCTION, &session_progress_handler);
-  #endif
-  curl_easy_setopt(state->handle, CURLOPT_PROGRESSDATA, state);
+  state->share = curl_share_init();
+  curl_share_setopt(state->share, CURLSHOPT_SHARE, CURL_LOCK_DATA_COOKIE);
+  curl_share_setopt(state->share, CURLSHOPT_SHARE, CURL_LOCK_DATA_DNS);
+  curl_share_setopt(state->share, CURLSHOPT_SHARE, CURL_LOCK_DATA_SSL_SESSION);
+  curl_share_setopt(state->share, CURLSHOPT_SHARE, CURL_LOCK_DATA_CONNECT);
+  curl_share_setopt(state->share, CURLSHOPT_SHARE, CURL_LOCK_DATA_PSL);
+  state->base_handle = curl_easy_init();
+  curl_easy_setopt(state->base_handle, CURLOPT_SHARE, state->share);
+  curl_easy_setopt(state->base_handle, CURLOPT_WRITEFUNCTION, &session_write_handler);
+  curl_easy_setopt(state->base_handle, CURLOPT_WRITEDATA, &state->body_buffer);
+  curl_easy_setopt(state->base_handle, CURLOPT_HEADERFUNCTION, &session_write_handler);
+  curl_easy_setopt(state->base_handle, CURLOPT_HEADERDATA, &state->header_buffer);
+  curl_easy_setopt(state->base_handle, CURLOPT_NOSIGNAL, 1);
+  curl_easy_setopt(state->base_handle, CURLOPT_NOPROGRESS, 0);
+#if LIBCURL_VERSION_NUM >= 0x072000
+  /* this is libCURLv7.32.0 or later, supports CURLOPT_XFERINFOFUNCTION */
+  curl_easy_setopt(state->base_handle, CURLOPT_XFERINFOFUNCTION, &session_progress_handler);
+#else
+  curl_easy_setopt(state->base_handle, CURLOPT_PROGRESSFUNCTION, &session_progress_handler);
+#endif
+  curl_easy_setopt(state->base_handle, CURLOPT_PROGRESSDATA, state);
+#ifdef CURLPROTO_HTTP
+  // Security: do not allow Curl to go looking on gopher/SMTP etc.
+  // Must prevent situations like this:
+  // https://hackerone.com/reports/115748
+  curl_easy_setopt(state->base_handle, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
+  curl_easy_setopt(state->base_handle, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
+#endif
 
   return obj;
 }
@@ -442,7 +460,7 @@ static void set_request_body(struct patron_curl_state* state, VALUE stringable_o
  */
 static void set_options_from_request(VALUE self, VALUE request) {
   struct patron_curl_state* state = get_patron_curl_state(self);
-  CURL* curl = state->handle;
+  CURL* curl = curl_easy_duphandle(state->base_handle);
 
   ID    action                = Qnil;
   VALUE headers               = Qnil;
@@ -463,6 +481,9 @@ static void set_options_from_request(VALUE self, VALUE request) {
   VALUE download_byte_limit   = rb_funcall(request, rb_intern("download_byte_limit"), 0);
   VALUE maybe_progress_proc   = rb_funcall(request, rb_intern("progress_callback"), 0);
 
+  state->handle = curl;
+  curl_easy_setopt(curl, CURLOPT_SHARE, state->share);
+
   if (RTEST(download_byte_limit)) {
     state->download_byte_limit = FIX2INT(download_byte_limit);
   } else {
@@ -484,7 +505,7 @@ static void set_options_from_request(VALUE self, VALUE request) {
   }
 
   action = SYM2ID(action_name);
-  if(rb_funcall(request, rb_intern("force_ipv4"), 0)) {
+  if (RTEST(rb_funcall(request, rb_intern("force_ipv4"), 0))) {
     curl_easy_setopt(curl, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4);
   }
   if (action == rb_intern("get")) {
@@ -587,13 +608,6 @@ static void set_options_from_request(VALUE self, VALUE request) {
   }
   curl_easy_setopt(curl, CURLOPT_URL, StringValuePtr(url));
 
-#ifdef CURLPROTO_HTTP
-  // Security: do not allow Curl to go looking on gopher/SMTP etc.
-  // Must prevent situations like this:
-  // https://hackerone.com/reports/115748
-  curl_easy_setopt(curl, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
-  curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
-#endif
 
   timeout = rb_funcall(request, rb_intern("timeout"), 0);
   if (RTEST(timeout)) {
@@ -731,11 +745,6 @@ static void set_options_from_request(VALUE self, VALUE request) {
   if (RTEST(buffer_size)) {
      curl_easy_setopt(curl, CURLOPT_BUFFERSIZE, NUM2LONG(buffer_size));
   }
-
-  if(state->debug_file) {
-    curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
-    curl_easy_setopt(curl, CURLOPT_STDERR, state->debug_file);
-  }
 }
 
 /* Use the info in a Curl handle to create a new Response object. */
@@ -750,10 +759,10 @@ static VALUE create_response(VALUE self, CURL* curl, VALUE header_buffer, VALUE
   args[0] = rb_str_new2(effective_url);
 
   curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &code);
-  args[1] = INT2NUM(code);
+  args[1] = LONG2NUM(code);
 
   curl_easy_getinfo(curl, CURLINFO_REDIRECT_COUNT, &count);
-  args[2] = INT2NUM(count);
+  args[2] = LONG2NUM(count);
 
   args[3] = header_buffer;
   args[4] = body_buffer;
@@ -796,37 +805,19 @@ void session_ubf_abort(void* patron_state) {
 static VALUE perform_request(VALUE self) {
   struct patron_curl_state *state = get_patron_curl_state(self);
   CURL* curl = state->handle;
-  membuffer* header_buffer = NULL;
-  membuffer* body_buffer = NULL;
   CURLcode ret = 0;
 
   state->interrupt = 0;            /* clear the interrupt flag */
 
-  header_buffer = &state->header_buffer;
-  body_buffer = &state->body_buffer;
-
-  membuffer_clear(header_buffer);
-  membuffer_clear(body_buffer);
-
-  /* headers */
-  curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, &session_write_handler);
-  curl_easy_setopt(curl, CURLOPT_HEADERDATA, header_buffer);
-
-  /* body */
-  if (!state->download_file) {
-    curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, &session_write_handler);
-    curl_easy_setopt(curl, CURLOPT_WRITEDATA, body_buffer);
-  }
-
   ret = (CURLcode) rb_thread_call_without_gvl(
           (void *(*)(void *)) curl_easy_perform, curl,
           session_ubf_abort, (void*)state
         );
 
   if (CURLE_OK == ret) {
-    VALUE header_str = membuffer_to_rb_str(header_buffer);
+    VALUE header_str = membuffer_to_rb_str(&state->header_buffer);
     VALUE body_str = Qnil;
-    if (!state->download_file) { body_str = membuffer_to_rb_str(body_buffer); }
+    if (!state->download_file) { body_str = membuffer_to_rb_str(&state->body_buffer); }
 
     curl_easy_setopt(curl, CURLOPT_COOKIELIST, "FLUSH"); // Flush cookies to the cookie jar
 
@@ -841,16 +832,19 @@ static VALUE perform_request(VALUE self) {
  */
 static VALUE cleanup(VALUE self) {
   struct patron_curl_state *state = get_patron_curl_state(self);
-  curl_easy_reset(state->handle);
+  curl_easy_cleanup(state->handle);
 
   if (state->headers) {
     curl_slist_free_all(state->headers);
     state->headers = NULL;
   }
+  membuffer_clear(&state->header_buffer);
 
   if (state->download_file) {
     fclose(state->download_file);
     state->download_file = NULL;
+  } else {
+    membuffer_clear(&state->body_buffer);
   }
 
   if (state->request_body_file) {
@@ -910,7 +904,7 @@ static VALUE session_interrupt(VALUE self) {
  */
 static VALUE add_cookie_file(VALUE self, VALUE file) {
   struct patron_curl_state *state = get_patron_curl_state(self);
-  CURL* curl = state->handle;
+  CURL* curl = state->base_handle;
   char* file_path = NULL;
 
   // FIXME: http://websystemsengineering.blogspot.nl/2013/03/curloptcookiefile-vs-curloptcookiejar.html
@@ -931,7 +925,8 @@ static VALUE add_cookie_file(VALUE self, VALUE file) {
  */
 static VALUE set_debug_file(VALUE self, VALUE file) {
   struct patron_curl_state *state = get_patron_curl_state(self);
-  char* file_path = RSTRING_PTR(file);
+  CURL *curl = state->base_handle;
+  char *file_path = RSTRING_PTR(file);
 
   session_close_debug_file(state);
 
@@ -940,6 +935,8 @@ static VALUE set_debug_file(VALUE self, VALUE file) {
   } else {
     state->debug_file = stderr;
   }
+  curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
+  curl_easy_setopt(curl, CURLOPT_STDERR, state->debug_file);
 
   return self;
 }

spec/session_spec.rb

@@ -262,7 +262,11 @@ def yaml_load(str)
       callback_args << [dltotal, dlnow, ultotal, ulnow]
     }
     session.get("/slow")
+    expect(callback_args).not_to be_empty
 
+    # Make sure that the callback setting was not reset.
+    callback_args = []
+    session.get("/slow")
     expect(callback_args).not_to be_empty
   end
 

spec/session_ssl_spec.rb

@@ -253,6 +253,16 @@ def yaml_load(str)
     expect(body.request_method).to be == "GET"
   end
 
+  it "should allow to specify insecure mode per-request" do
+    @session.insecure = false
+    expect {
+      @session.request(:get, "/test", {}, insecure: true)
+    }.not_to raise_error
+    expect {
+      @session.request(:get, "/test", {})
+    }.to raise_error(Patron::Error)
+  end
+
   it "should work with different SSL versions" do
     ['TLSv1_0','TLSv1_1'].each do |version|
       @session.ssl_version = version