Re: [mule-scm] [mule][24959] branches/mule-3.1.x/transports/http/src: MULE-6491: HTTP/ S transport does not reuse connections

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [mule-scm] [mule][24959] branches/mule-3.1.x/transports/http/src: MULE-6491: HTTP/ S transport does not reuse connections

Daniel Feist
Review:

i) Why isn't this implemented in HttpClientMessageDispatcher? Also won't this issue still exist with http otherwise?

ii) According to Mule code style guide.  PROTOCOL should only be used for a field that is a constant.

iii) Is it important that only one Protocol instance is created?

- If it is important then the getProtocol() method needs improving to ensure only a single instance is ever created.
- If it is not important if more than one protocol is created you don't really need a synchronised collection, although I don't see it as significant issues.

iv) Ideally test method has a more meaningful name related to what you are testing.


Dan


On Oct 18, 2012, at 11:55 PM, [hidden email] wrote:

[mule][24959] branches/mule-3.1.x/transports/http/src: MULE-6491: HTTP/S transport does not reuse connections
Revision
24959
Author
svacas
Date
2012-10-18 16:55:25 -0500 (Thu, 18 Oct 2012)

Log Message

MULE-6491: HTTP/S transport does not reuse connections

-cache protocol instances so connection can be reused

Modified Paths

  • <a href="x-msg://380/#branchesmule31xtransportshttpsrcmainjavaorgmuletransporthttpHttpsClientMessageDispatcherjava">branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java

Added Paths

  • <a href="x-msg://380/#branchesmule31xtransportshttpsrctestjavaorgmuletransporthttpHttpsClientMessageDispatcherTestCasejava">branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java

Diff

Modified: branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java (24958 => 24959)


--- branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java	2012-10-18 15:05:55 UTC (rev 24958)
+++ branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java	2012-10-18 21:55:25 UTC (rev 24959)
@@ -13,6 +13,10 @@
 import org.mule.api.endpoint.OutboundEndpoint;
 
 import java.net.URI;
+import java.security.GeneralSecurityException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 
 import javax.net.ssl.SSLSocketFactory;
 
@@ -22,7 +26,9 @@
 
 public class HttpsClientMessageDispatcher extends HttpClientMessageDispatcher
 {
-    
+
+    private final Map<String, Protocol> PROTOCOL = Collections.synchronizedMap(new HashMap<String, Protocol>());
+
     public HttpsClientMessageDispatcher(OutboundEndpoint endpoint)
     {
         super(endpoint);
@@ -32,19 +38,28 @@
     protected HostConfiguration getHostConfig(URI uri) throws Exception
     {        
         HostConfiguration hostConfig = new MuleHostConfiguration(super.getHostConfig(uri));
-
-        HttpsConnector httpsConnector = (HttpsConnector) httpConnector;
-        SSLSocketFactory factory = httpsConnector.getSslSocketFactory();
-        ProtocolSocketFactory protocolSocketFactory = new MuleSecureProtocolSocketFactory(factory);
-        Protocol protocol = new Protocol(uri.getScheme().toLowerCase(), protocolSocketFactory, 443);
-        
         String host = uri.getHost();
         int port = uri.getPort();
+        Protocol protocol = getProtocol(uri.getScheme().toLowerCase());
         hostConfig.setHost(host, port, protocol);            
         
         return hostConfig;
     }
 
+    private Protocol getProtocol(String scheme) throws GeneralSecurityException
+    {
+        Protocol protocol = PROTOCOL.get(scheme);
+        if (protocol == null)
+        {
+            HttpsConnector httpsConnector = (HttpsConnector) httpConnector;
+            SSLSocketFactory factory = httpsConnector.getSslSocketFactory();
+            ProtocolSocketFactory protocolSocketFactory = new MuleSecureProtocolSocketFactory(factory);
+            protocol = new Protocol(scheme, protocolSocketFactory, 443);
+            PROTOCOL.put(scheme, protocol);
+        }
+        return protocol;
+    }
+
 }
 
 

Added: branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java (0 => 24959)


--- branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java	                        (rev 0)
+++ branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java	2012-10-18 21:55:25 UTC (rev 24959)
@@ -0,0 +1,38 @@
+/*
+ * $Id$
+ * --------------------------------------------------------------------------------------
+ * Copyright (c) MuleSoft, Inc.  All rights reserved.  http://www.mulesoft.com
+ *
+ * The software in this package is published under the terms of the CPAL v1.0
+ * license, a copy of which has been included with this distribution in the
+ * LICENSE.txt file.
+ */
+package org.mule.transport.http;
+
+import org.mule.api.endpoint.OutboundEndpoint;
+import org.mule.api.transport.Connector;
+
+import java.net.URI;
+
+import org.apache.commons.httpclient.HostConfiguration;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+public class HttpsClientMessageDispatcherTestCase
+{
+
+    @Test
+    public void getHost() throws Exception
+    {
+        OutboundEndpoint oe = Mockito.mock(OutboundEndpoint.class);
+        Connector connector = Mockito.mock(HttpsConnector.class);
+        Mockito.when(oe.getConnector()).thenReturn(connector);
+        HttpsClientMessageDispatcher dispatcher = new HttpsClientMessageDispatcher(oe);
+
+        URI uri = new URI("https://www.mulesoft.org/");
+        HostConfiguration hc1 = dispatcher.getHostConfig(uri);
+        HostConfiguration hc2 = dispatcher.getHostConfig(uri);
+        Assert.assertEquals(hc1, hc2);
+    }
+}
Property changes on: branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java
___________________________________________________________________

Added: svn:keywords

Added: svn:eol-style


To unsubscribe from this list please visit:

http://xircles.codehaus.org/manage_email


Reply | Threaded
Open this post in threaded view
|

Re: [mule-scm] [mule][24959] branches/mule-3.1.x/transports/http/src: MULE-6491: HTTP/ S transport does not reuse connections

Santiago Vacas
On Wed, Oct 24, 2012 at 1:09 PM, Daniel Feist <[hidden email]> wrote:
> Review:
>
> i) Why isn't this implemented in HttpClientMessageDispatcher? Also won't
> this issue still exist with http otherwise?

HttpClientMessageDispatcher is using
org.apache.commons.httpclient.protocol.Protocol.getProtocol(...) that
does the caching. In the case of Https it is using a custom
SocketFactory so the protocol is created in a different way.

>
> ii) According to Mule code style guide.  PROTOCOL should only be used for a
> field that is a constant.

will change to lowercase

>
> iii) Is it important that only one Protocol instance is created?
>
> - If it is important then the getProtocol() method needs improving to ensure
> only a single instance is ever created.
> - If it is not important if more than one protocol is created you don't
> really need a synchronised collection, although I don't see it as
> significant issues.

in order to reuse the connections the protocol instances should be the
same, but there's no harm if two or more are created at the beginning,
eventually the same one will be used from that point onwards. The map
needs to be synchronized as different threads may be modifying it
structurally (puts).

>
> iv) Ideally test method has a more meaningful name related to what you are
> testing.

ack

>
>
> Dan
>
>
> On Oct 18, 2012, at 11:55 PM, [hidden email] wrote:
>
> Revision 24959 Author svacas Date 2012-10-18 16:55:25 -0500 (Thu, 18 Oct
> 2012)
>
> Log Message
>
> MULE-6491: HTTP/S transport does not reuse connections
>
> -cache protocol instances so connection can be reused
>
> Modified Paths
>
> branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java
>
> Added Paths
>
> branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java
>
> Diff
>
> Modified:
> branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java
> (24958 => 24959)
>
> ---
> branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java 2012-10-18
> 15:05:55 UTC (rev 24958)
> +++
> branches/mule-3.1.x/transports/http/src/main/java/org/mule/transport/http/HttpsClientMessageDispatcher.java 2012-10-18
> 21:55:25 UTC (rev 24959)
> @@ -13,6 +13,10 @@
>  import org.mule.api.endpoint.OutboundEndpoint;
>
>  import java.net.URI;
> +import java.security.GeneralSecurityException;
> +import java.util.Collections;
> +import java.util.HashMap;
> +import java.util.Map;
>
>  import javax.net.ssl.SSLSocketFactory;
>
> @@ -22,7 +26,9 @@
>
>  public class HttpsClientMessageDispatcher extends
> HttpClientMessageDispatcher
>  {
> -
> +
> +    private final Map<String, Protocol> PROTOCOL =
> Collections.synchronizedMap(new HashMap<String, Protocol>());
> +
>      public HttpsClientMessageDispatcher(OutboundEndpoint endpoint)
>      {
>          super(endpoint);
> @@ -32,19 +38,28 @@
>      protected HostConfiguration getHostConfig(URI uri) throws Exception
>      {
>          HostConfiguration hostConfig = new
> MuleHostConfiguration(super.getHostConfig(uri));
> -
> -        HttpsConnector httpsConnector = (HttpsConnector) httpConnector;
> -        SSLSocketFactory factory = httpsConnector.getSslSocketFactory();
> -        ProtocolSocketFactory protocolSocketFactory = new
> MuleSecureProtocolSocketFactory(factory);
> -        Protocol protocol = new Protocol(uri.getScheme().toLowerCase(),
> protocolSocketFactory, 443);
> -
>          String host = uri.getHost();
>          int port = uri.getPort();
> +        Protocol protocol = getProtocol(uri.getScheme().toLowerCase());
>          hostConfig.setHost(host, port, protocol);
>
>          return hostConfig;
>      }
>
> +    private Protocol getProtocol(String scheme) throws
> GeneralSecurityException
> +    {
> +        Protocol protocol = PROTOCOL.get(scheme);
> +        if (protocol == null)
> +        {
> +            HttpsConnector httpsConnector = (HttpsConnector) httpConnector;
> +            SSLSocketFactory factory =
> httpsConnector.getSslSocketFactory();
> +            ProtocolSocketFactory protocolSocketFactory = new
> MuleSecureProtocolSocketFactory(factory);
> +            protocol = new Protocol(scheme, protocolSocketFactory, 443);
> +            PROTOCOL.put(scheme, protocol);
> +        }
> +        return protocol;
> +    }
> +
>  }
>
>
>
> Added:
> branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java
> (0 => 24959)
>
> ---
> branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java
> (rev 0)
> +++
> branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java 2012-10-18
> 21:55:25 UTC (rev 24959)
> @@ -0,0 +1,38 @@
> +/*
> + * $Id$
> + *
> --------------------------------------------------------------------------------------
> + * Copyright (c) MuleSoft, Inc.  All rights reserved.
> http://www.mulesoft.com
> + *
> + * The software in this package is published under the terms of the CPAL
> v1.0
> + * license, a copy of which has been included with this distribution in the
> + * LICENSE.txt file.
> + */
> +package org.mule.transport.http;
> +
> +import org.mule.api.endpoint.OutboundEndpoint;
> +import org.mule.api.transport.Connector;
> +
> +import java.net.URI;
> +
> +import org.apache.commons.httpclient.HostConfiguration;
> +import org.junit.Assert;
> +import org.junit.Test;
> +import org.mockito.Mockito;
> +
> +public class HttpsClientMessageDispatcherTestCase
> +{
> +
> +    @Test
> +    public void getHost() throws Exception
> +    {
> +        OutboundEndpoint oe = Mockito.mock(OutboundEndpoint.class);
> +        Connector connector = Mockito.mock(HttpsConnector.class);
> +        Mockito.when(oe.getConnector()).thenReturn(connector);
> +        HttpsClientMessageDispatcher dispatcher = new
> HttpsClientMessageDispatcher(oe);
> +
> +        URI uri = new URI("https://www.mulesoft.org/");
> +        HostConfiguration hc1 = dispatcher.getHostConfig(uri);
> +        HostConfiguration hc2 = dispatcher.getHostConfig(uri);
> +        Assert.assertEquals(hc1, hc2);
> +    }
> +}
> Property changes on:
> branches/mule-3.1.x/transports/http/src/test/java/org/mule/transport/http/HttpsClientMessageDispatcherTestCase.java
> ___________________________________________________________________
>
> Added: svn:keywords
>
> Added: svn:eol-style
>
> ________________________________
>
> To unsubscribe from this list please visit:
>
> http://xircles.codehaus.org/manage_email
>
>

---------------------------------------------------------------------
To unsubscribe from this list, please visit:

    http://xircles.codehaus.org/manage_email