[mule-dev] 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, sva...@codehaus.org 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

[Santiago Vacas]

On Wed, Oct 24, 2012 at 1:09 PM, Daniel Feist <dfe...@gmail.com> 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

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

http://xircles.codehaus.org/manage_email