axis-java-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Amila Suriarachchi <amilasuriarach...@gmail.com>
Subject Re: svn commit: r992877 - in /axis/axis2/java/core/trunk: ./ modules/adb-codegen/test-resources/testsuite/ modules/adb-codegen/test/org/apache/axis2/schema/particlemaxoccurs/ modules/kernel/src/org/apache/axis2/jsr181/ modules/kernel/src/org/apache/a
Date Sat, 18 Sep 2010 07:15:09 GMT
On Fri, Sep 17, 2010 at 8:36 PM, Jarek Gawor <jgawor@gmail.com> wrote:

> On Fri, Sep 17, 2010 at 7:55 AM, Amila Suriarachchi
> <amilasuriarachchi@gmail.com> wrote:
> >
> >
> >>
> >> See https://issues.apache.org/jira/browse/AXIS2-4751 for details. The
> >> thread safety problem was solved by not reusing HttpClient instance.
> >
> > As I told, this bug is reported with Axis2 1.5.1 not with trunk. hence
> this
> > issue is not relevant.
> >
> > this is the original code.
> > 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);
> >             }
> >         }
> >
> >         // Get the timeout values set in the runtime
> >         initializeTimeouts(msgContext, httpClient);
> >
> >         return httpClient;
> >
> > it reuses http client only if user has set so. otherwise if the user has
> set
> > an http connection manager object then
> > it creates a new httpClient per every call.
>
> This code is bad for two reasons:
>
> 1) In the first block when REUSE_HTTP_CLIENT is true, the code will
> reuse HttpClient instance. As I mentioned before, that way we use
> HttpClient instance in the code right now is not thread safe.
>

Can you please explain how one can not come up with the same argument with
the current code?
Here people reuse httpClient if they know it is thread safe.

>
> This directly relates to the key problem my original changes were
> addressing.
>

Please see here[1]. What you are talking about an issue due to the changes
done only to Axis2 1.5.1.


> 2) When REUSE_HTTP_CLIENT is not set or false and
> MULTITHREAD_HTTP_CONNECTION_MANAGER is not set, the code will create a
> new instance of MultiThreadedHttpConnectionManager and HttpClient.
> Creating a new instance of MultiThreadedHttpConnectionManager on each
> call means it is creating a new connection pool per call but the
> connection pool is not really being reused. That can lead to a lot of
> connections being open and remain open longer then needed.
>

In Axis2 1.5 there are three possible methods in which http client is used.

1. Creating one connection per invocation
2. Reusing same httpClient object per all invocations
3. Reusing same connectionManager object for all invocations.

in Axis2 1.5.1 what is has done is actually restricting this to option 2.
Hence your issue.

Then the current code has restricted it to only option 2 and 3 and make
option 3 default.

In Axis2 1.5 the default is option 1. Which has the problems you mentioned
at the high loads. For normal
loads that is not a considerable problem. For such case users has option to
move into option 2 or 3 according to
their usage.

So what we try to say making option 3 as default (and removing options 1) is
that it supports high loads by default. But it is not the case as I shown
since it creates only two connections to the back end servers.


> An attempt was made in the 1.5 branch to ensure connection pool is
> reused between the calls but that reused HttpClient instance instead
> of HttpConnectionManager instance... and you know the rest.
>
> So, please do not revert back to this version of the code.
>

Even we agree to keep only option 2 and 3 (default) I see following problems
with this patch.

1.  if (httpClient == null) {
            httpClient = (HttpClient)
configContext.getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
        }
No need to have this code. (yes this is there with the earlier code as well)
when you check with message context it
should have return the property if it in the configuration context.

2. if (connManager == null) {
            // reuse HttpConnectionManager
            synchronized (configContext) {
                connManager = (HttpConnectionManager)
configContext.getProperty(

HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER);
                if (connManager == null) {
                    log.trace("Making new ConnectionManager");
                    connManager = new MultiThreadedHttpConnectionManager();

configContext.setProperty(HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER,

                                              connManager);
                }
            }
        }

Instead of doing this we need to set the connection manager at the transport
initialization time. We may be creating an unnecessary connection manage if
user always reuse the httpClient. But I think it is better than having a
synchronized block.

3. // Set the default timeout in case we have a connection pool starvation
to 30sec
        httpClient.getParams().setConnectionManagerTimeout(30000);

remove this and ask users to must use serviceClient.cleanupTransport() since
option 2 and 3 requires this. otherwise connections are not released and the
system hangs.

thanks,
Amila.

[1]
http://svn.apache.org/viewvc/axis/axis2/java/core/branches/1_5/modules/transport/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java?revision=824340&view=markup


>
> Jarek
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: java-dev-unsubscribe@axis.apache.org
> For additional commands, e-mail: java-dev-help@axis.apache.org
>
>


-- 
Amila Suriarachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/

Mime
View raw message