You are viewing a plain text version of this content. The canonical link for it is here.
Posted to java-dev@axis.apache.org by Amila Suriarachchi <am...@gmail.com> on 2010/07/24 08:59:16 UTC
Re: svn commit: r963710 - /axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java
On Tue, Jul 13, 2010 at 7:02 PM, <ga...@apache.org> wrote:
> Author: gawor
> Date: Tue Jul 13 13:32:24 2010
> New Revision: 963710
>
> URL: http://svn.apache.org/viewvc?rev=963710&view=rev
> Log:
> AXIS2-4751: Create a new instance of HttpClient since it is not used in
> thread-safe way. The connection manager is not automatically reused right
> now because other bugs in Axis2 or related libraries cause test failures
> when connection reuse is enabled.
>
the AXIS2-4751 is reported against the Axis2 1.5.1 and hence I think it is
not relevant with Axis2 trunk.
Axis2 trunk does not have the httpClient fix with has done to Axis2 1.5.1.
Hence it does not reuse the httpClient objects unless user have asked to do
so. This is true for Http Connection manager as well. If user wishes to have
a single conneciton manager it can set it.
AFAIK this defualt creating new httpcliet objects for each request works for
most of the cases. But I may cause CLOSE_WAIT issue with high loads. In this
case users can either reuse HttpClient or only HttpConneciton manager
according to their senarios. Acutally Axis2 1.5.1 fix just force people to
reuse one httpClient object.
So I think current trunk is ok. Infact I think even your change does not
make any real change compared to trunk version which was there. But I agree
with you that using two variables REUSE_HTTP_CLIENT and CACHED_HTTP_CLIENT
is not required and could have done with one variable.
Please see below for some comments of your patch.
>
> Modified:
>
> axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java
>
> Modified:
> axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java
> URL:
> http://svn.apache.org/viewvc/axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java?rev=963710&r1=963709&r2=963710&view=diff
>
> ==============================================================================
> ---
> axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java
> (original)
> +++
> axis/axis2/java/core/trunk/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java
> Tue Jul 13 13:32:24 2010
> @@ -25,12 +25,12 @@ import org.apache.axiom.om.OMOutputForma
> import org.apache.axis2.AxisFault;
> import org.apache.axis2.Constants;
> import org.apache.axis2.context.MessageContext;
> +import org.apache.axis2.context.ConfigurationContext;
> import org.apache.axis2.context.OperationContext;
> import org.apache.axis2.description.TransportOutDescription;
> import org.apache.axis2.i18n.Messages;
> import org.apache.axis2.transport.MessageFormatter;
> import org.apache.axis2.transport.TransportUtils;
> -import org.apache.axis2.util.JavaUtils;
> import org.apache.axis2.wsdl.WSDLConstants;
> import org.apache.commons.httpclient.Credentials;
> import org.apache.commons.httpclient.Header;
> @@ -485,44 +485,55 @@ public abstract class AbstractHTTPSender
> }
>
> protected HttpClient getHttpClient(MessageContext msgContext) {
> - HttpClient httpClient;
> - Object reuse =
> msgContext.getOptions().getProperty(HTTPConstants.REUSE_HTTP_CLIENT);
> - if (reuse == null) {
> - reuse =
> msgContext.getConfigurationContext().getProperty(HTTPConstants.REUSE_HTTP_CLIENT);
> - }
> - if (reuse != null && JavaUtils.isTrueExplicitly(reuse)) {
> - httpClient = (HttpClient)
> msgContext.getOptions().getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
> - if (httpClient == null) {
> - httpClient = (HttpClient)
> msgContext.getConfigurationContext()
> - .getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
> - }
> - if (httpClient != null)
> - return httpClient;
> - MultiThreadedHttpConnectionManager connectionManager =
> - new MultiThreadedHttpConnectionManager();
> - httpClient = new HttpClient(connectionManager);
> - msgContext.getConfigurationContext()
> - .setProperty(HTTPConstants.CACHED_HTTP_CLIENT,
> httpClient);
> - } else {
> - HttpConnectionManager connManager =
> - (HttpConnectionManager) msgContext.getProperty(
> -
> HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER);
> - if (connManager == null) {
> - connManager =
> - (HttpConnectionManager) msgContext.getProperty(
> -
> HTTPConstants.MUTTITHREAD_HTTP_CONNECTION_MANAGER);
> - }
> - if(connManager != null){
> - httpClient = new HttpClient(connManager);
> - } else {
> - //Multi threaded http connection manager has set as the
> default
> - connManager = new MultiThreadedHttpConnectionManager();
> - httpClient = new HttpClient(connManager);
> + ConfigurationContext configContext =
> msgContext.getConfigurationContext();
> +
> + HttpClient httpClient = (HttpClient)
> msgContext.getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
> + if (httpClient == null) {
> + httpClient = (HttpClient)
> configContext.getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
>
if the configuration context contain this property it should have return
even with the messagecontext.getproperty. According to what was there first
thing should be getOptions.getProperty.
> + }
> + if (httpClient != null) {
> + return httpClient;
> + }
> +
> + HttpConnectionManager connManager =
> + (HttpConnectionManager) msgContext.getProperty(
> + HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER);
> + if (connManager == null) {
> + connManager =
> + (HttpConnectionManager) msgContext.getProperty(
> + HTTPConstants.MUTTITHREAD_HTTP_CONNECTION_MANAGER);
> + }
> + if (connManager == null) {
> + // reuse HttpConnectionManager
> + synchronized (configContext) {
> + connManager = (HttpConnectionManager)
> configContext.getProperty(
> +
> HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER);
>
Again I think no need to check in configuratin context if the property is
not in messageContext.
+ if (connManager == null) {
> + log.trace("Making new ConnectionManager");
> + connManager = new
> MultiThreadedHttpConnectionManager();
> + /*
> + * Commented out for now as bugs in other parts of
> Axis2 cause test failures when
> + * proper connection reuse is enabled.
> + */
> + //
> configContext.setProperty(HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER,
> + // connManager);
>
if you do not set this I think no need to have the synchronize block.
thanks,
Amila.
> +
> + }
> }
> }
>
> + /*
> + * Create a new instance of HttpClient since it is not
> + * used in thread-safe way here.
> + */
> + httpClient = new HttpClient(connManager);
> +
> + // Set the default timeout in case we have a connection pool
> starvation to 30sec
> + httpClient.getParams().setConnectionManagerTimeout(30000);
> +
> // Get the timeout values set in the runtime
> initializeTimeouts(msgContext, httpClient);
> +
> return httpClient;
> }
>
>
>
>
--
Amila Suriarachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/