[PATCH] Persistent connections and HTTPS

This is a multi-part message in MIME format.
--------------090609080307010009030309
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Hi,

Attached patch implements a workaround to allow persistent connections to work with HTTPS,
avoiding bugs in Https_client#continue. See below for the long explanation.

Persistent HTTP connections work if I enable the aggressive connection cache with Http_client.
However with Https_client I noticed that it always closed and reopened the Ssl connections. 
While this doesn't influence the application's correctness (thanks to the retry mechanisms in Http_client!), it heavily influences its performance: the latency is very bad due to repeated reopened connections, and repeated SSL handshakes ... even on localhost.

Enabling Netlog debugging showed that some Ssl exceptions were thrown each time around read/write/shutdown after the 1st query completed. Further investigation revealed that the problem might be with Https_client#continue:

       method continue fd cb tmo tmo_x host port esys =
-	let mplex =
-	  Uq_ssl.create_ssl_multiplex_controller
-	    ~close_inactive_descr:true
-	    ~preclose:(preclose fd)
-	    ~initial_state:`Client
-	    ~timeout:(tmo, tmo_x)
-	    fd ctx esys in
-	(mplex :> Uq_engines.multiplex_controller)

Problems with this code:
  * creating a new ssl multiplex controller will create a new Ssl.socket (and share just the Ssl.context and fd).
  * the new Ssl.socket doesn't have a defined state (SSL_set_connected_state was not called, and there was no prior handshake either), causing further operations on it to raise errors (if I add some debugging code
to Uq_ssl to print Ssl.get_error_string):
[Thu Sep 19 14:57:29 2013] [debug] [7534:11] Uq_ssl: SSL write error: error:140D0114:SSL routines:SSL_write:uninitialized
[Thu Sep 19 14:57:29 2013] [debug] [7534:11] Uq_ssl: SSL read error: error:140DF114:SSL routines:SSL_read:uninitialized
[Thu Sep 19 14:57:29 2013] [debug] [7534:11] Uq_ssl: SSL shutdown error: error:140E0114:SSL routines:SSL_shutdown:uninitialized
  * the new Ssl.socket is probably missing the state of the handshake (session keys, etc.)

    
The attached workaround implements a very simple workaround: if the esys is still the same
just reuse the previous multiplex controller, I'm guessing this is what the Hashtbl was meant for anyway.

A better solution would be to have something like create_ssl_multiplex_controller_for_existing_session that would take the existing Ssl.socket instead of creating a new one, and then Https_client#continue wouldn't have to drop the connection if esys or tmo changes.

Best regards,
--Edwin


--------------090609080307010009030309
Content-Type: text/x-patch;
 name="https_client.patch"
Content-Disposition: attachment;
 filename="https_client.patch"
Content-Transfer-Encoding: quoted-printable

Author: T=C3=B6r=C3=B6k Edwin <[email protected]>

    Make persistent connections work with HTTPS
   =20
    When reusing a connection (as with the aggressive connection cache)
    Https_client#continue is called, which creates a new Ssl.socket (from=
 same Ssl.context and fd).
    This raises exceptions during read/write/shutdown, because the newly =
created Ssl.socket:
     * didn't have the handshake details from the already established Ssl=
.socket (session key, etc.)
     * SSL_set_connected_state was not called, so SSL_read/SSL_write/SSL_=
shutdown returned 'uninit'
   =20
    The result was that persistent HTTPS connections always raised an Ssl=
-related
    exception on the 2nd query, which caused the connection to be dropped=
, a new
    connection established and the query retried. This didn't influence t=
he
    application's correctness (the query eventually got answered), but it=
 did
    influence its performance (latency was very bad due to the repeated
    reopened connections, and repeated SSL handshakes).
   =20
    The workaround implemented here simply reuses the old ssl multiplex c=
ontroller
    if the event system is still the same.
    One problem with this approach is that the timeout cannot be changed =
in
     #continue, and changing the event system causes the connection to be=
 dropped as
    before.
   =20
    A better solution would be to have something like
    create_ssl_multiplex_controller_for_existing_session that would take
    the existing Ssl.socket instead of creating a new one.

diff --git a/https_client.ml b/https_client.ml
index 7f7abfb..0d4df30 100644
--- a/https_client.ml
+++ b/https_client.ml
@@ -64,13 +64,22 @@ let https_transport_channel_type ?(verify =3D fun _ _=
 _ -> ())
=20
=20
       method continue fd cb tmo tmo_x host port esys =3D
-	let mplex =3D
-	  Uq_ssl.create_ssl_multiplex_controller
-	    ~close_inactive_descr:true
-	    ~preclose:(preclose fd)
-	    ~initial_state:`Client
-	    ~timeout:(tmo, tmo_x)
-	    fd ctx esys in
-	(mplex :> Uq_engines.multiplex_controller)
+        let mplex =3D Hashtbl.find ctx_of_fd fd in
+        (* if not found: Not_found causes new connection to be opened *)
+        if mplex#event_system =3D=3D esys then
+          (* common case: reuse existing SSL connection *)
+          (mplex :> Uq_engines.multiplex_controller)
+        else begin
+          (* need to create new SSL connection on new esys.
+           * NB. We cannot just call create_ssl_multiplex_controller bec=
ause
+           * we'd be missing the SSL connection's state (session keys, e=
tc.),
+           * and also without SSL_set_connect_state SSL_read/SSL_write w=
ould
+           * fail with 'uninitialized'.
+           * We'd need something like
+           * create_ssl_multiplex_controller_for_existing_session.
+           * *)
+          mplex#inactivate ();
+          raise Not_found
+        end
     end
   )

--------------090609080307010009030309
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

------------------------------------------------------------------------------
LIMITED TIME SALE - Full Year of Microsoft Training For Just $49.99!
1,500+ hours of tutorials including VisualStudio 2012, Windows 8, SharePoint
2013, SQL 2012, MVC 4, more. BEST VALUE: New Multi-Library Power Pack includes
Mobile, Cloud, Java, and UX Design. Lowest price ever! Ends 9/22/13. 
http://pubads.g.doubleclick.net/gampad/clk?id=64545871&iu=/4140/ostg.clktrk
--------------090609080307010009030309
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

_______________________________________________
Ocamlnet-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ocamlnet-devel

--------------090609080307010009030309--