Return-Path: Delivered-To: apmail-axis-java-dev-archive@www.apache.org Received: (qmail 82582 invoked from network); 18 Sep 2010 07:15:41 -0000 Received: from unknown (HELO mail.apache.org) (140.211.11.3) by 140.211.11.9 with SMTP; 18 Sep 2010 07:15:41 -0000 Received: (qmail 1903 invoked by uid 500); 18 Sep 2010 07:15:40 -0000 Delivered-To: apmail-axis-java-dev-archive@axis.apache.org Received: (qmail 99825 invoked by uid 500); 18 Sep 2010 07:15:38 -0000 Mailing-List: contact java-dev-help@axis.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: java-dev@axis.apache.org Delivered-To: mailing list java-dev@axis.apache.org Received: (qmail 99817 invoked by uid 99); 18 Sep 2010 07:15:37 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 18 Sep 2010 07:15:37 +0000 X-ASF-Spam-Status: No, hits=2.2 required=10.0 tests=FREEMAIL_FROM,HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_PASS,T_TO_NO_BRKTS_FREEMAIL X-Spam-Check-By: apache.org Received-SPF: pass (athena.apache.org: domain of amilasuriarachchi@gmail.com designates 74.125.82.51 as permitted sender) Received: from [74.125.82.51] (HELO mail-ww0-f51.google.com) (74.125.82.51) by apache.org (qpsmtpd/0.29) with ESMTP; Sat, 18 Sep 2010 07:15:31 +0000 Received: by wwb28 with SMTP id 28so3009752wwb.32 for ; Sat, 18 Sep 2010 00:15:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:content-type; bh=Jc+a/ERdhSzoLnroeIIB4DQmSIEbBwepY3qOrSaTGbI=; b=hqQ9EnC9Upze5hpUJeir3BE7sv5il08jAvAuN5u8Dvh+kYBtYUeR13Gpf+ZyF6ROef LkqzvQmk6SMNlW0R6IE5Angbo0zj0sK2kH49rXq8foxJwVkmRfwBYYQeSgb7JwRHSoXD nNmn9mRAyIHhEuiIIZYN1mL87v0gRwD7+a1bY= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=DfXZifsccnXodad6PMHXf60KhbAZYdw9WhMovrhPyWTFr9n19jr1D15YI3mVlXsN5P pLnQN4P+BrXezu3b1xhOCsxTqED7dakOXrAYJDouoTu6y5LIuVwQRrvOsgOEQqkEECux 8nEO396bs+Cy2TYyvhxv9z3lHnAKm3gkwhh70= MIME-Version: 1.0 Received: by 10.216.11.205 with SMTP id 55mr5205595wex.51.1284794109336; Sat, 18 Sep 2010 00:15:09 -0700 (PDT) Received: by 10.227.43.5 with HTTP; Sat, 18 Sep 2010 00:15:09 -0700 (PDT) In-Reply-To: References: Date: Sat, 18 Sep 2010 12:45:09 +0530 Message-ID: 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 From: Amila Suriarachchi To: java-dev@axis.apache.org Content-Type: multipart/alternative; boundary=0016364d2cbd7f1ba004908370df --0016364d2cbd7f1ba004908370df Content-Type: text/plain; charset=ISO-8859-1 On Fri, Sep 17, 2010 at 8:36 PM, Jarek Gawor wrote: > On Fri, Sep 17, 2010 at 7:55 AM, Amila Suriarachchi > 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/ --0016364d2cbd7f1ba004908370df Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Fri, Sep 17, 2010 at 8:36 PM, Jarek G= awor <jgawor@gmail= .com> wrote:
On Fri, Sep 17, 2010 at 7:55 AM, Amila Su= riarachchi
<amilasuriarachchi@gmail.= com> wrote:
>
>
>>
>> See https://issues.apache.org/jira/browse/AXIS2-4751 for d= etails. The
>> thread safety problem was solved by not reusing HttpClient instanc= e.
>
> 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;
> =A0=A0=A0=A0=A0=A0=A0 Object reuse =3D
> msgContext.getOptions().getProperty(HTTPConstants.REUSE_HTTP_CLIENT);<= br> > =A0=A0=A0=A0=A0=A0=A0 if (reuse =3D=3D null) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 reuse =3D
> msgContext.getConfigurationContext().getProperty(HTTPConstants.REUSE_H= TTP_CLIENT);
> =A0=A0=A0=A0=A0=A0=A0 }
>
> =A0=A0=A0=A0=A0=A0=A0 if (reuse !=3D null && JavaUtils.isTrueE= xplicitly(reuse)) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 httpClient =3D (HttpClient)
> msgContext.getOptions().getProperty(HTTPConstants.CACHED_HTTP_CLIENT);=
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (httpClient =3D=3D null) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 httpClient =3D (HttpClie= nt)
> msgContext.getConfigurationContext()
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = .getProperty(HTTPConstants.CACHED_HTTP_CLIENT);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (httpClient !=3D null)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 return httpClient;
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 MultiThreadedHttpConnectionManager c= onnectionManager =3D
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 new MultiThr= eadedHttpConnectionManager();
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 httpClient =3D new HttpClient(connec= tionManager);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 msgContext.getConfigurationContext()=
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 .setProperty= (HTTPConstants.CACHED_HTTP_CLIENT,
> httpClient);
> =A0=A0=A0=A0=A0=A0=A0 } else {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 HttpConnectionManager connManager = =3D
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 (HttpConnect= ionManager) msgContext.getProperty(
>
> HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (connManager =3D=3D null) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 connManager =3D
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 = (HttpConnectionManager) msgContext.getProperty(
>
> HTTPConstants.MUTTITHREAD_HTTP_CONNECTION_MANAGER);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (connManager !=3D null) {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 httpClient =3D new HttpC= lient(connManager);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 } else {
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 //Multi threaded http co= nnection manager has set as the
> default
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 connManager =3D new Mult= iThreadedHttpConnectionManager();
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 httpClient =3D new HttpC= lient(connManager);
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
> =A0=A0=A0=A0=A0=A0=A0 }
>
> =A0=A0=A0=A0=A0=A0=A0 // Get the timeout values set in the runtime
> =A0=A0=A0=A0=A0=A0=A0 initializeTimeouts(msgContext, httpClient);
>
> =A0=A0=A0=A0=A0=A0=A0 return httpClient;
>
> it reuses http client only if user has set so. otherwise if the user h= as 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 ar= gument 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 addressin= g.

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 sa= me httpClient object per all invocations
3. Reusing same connectionManager object for all invocations.

in Axi= s2 1.5.1 what is has done is actually restricting this to option 2. Hence y= our 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 mentio= ned at the high loads. For normal
loads that is not a considerable prob= lem. 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 no= t 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 followin= g problems with this patch.

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

2. i= f (connManager =3D=3D null) {
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 // reuse HttpConnectionManager
=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0 synchronized (configContext) {
=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 connManager =3D (HttpConnectionManager) c= onfigContext.getProperty(
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 HTTPConstants.MUL= TITHREAD_HTTP_CONNECTION_MANAGER);
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 if (connManager =3D=3D null) = {
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 log.trace(&q= uot;Making new ConnectionManager");
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0 connManager =3D new MultiThreadedHttpConnection= Manager();
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 con= figContext.setProperty(HTTPConstants.MULTITHREAD_HTTP_CONNECTION_MANAGER, <= br> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 connManager);<= br>=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 }
=A0=A0=A0=A0=A0=A0=A0= =A0=A0=A0=A0 }
=A0=A0=A0=A0=A0=A0=A0 }

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

3. // Set the default timeout in case we have a connection pool starvat= ion to 30sec
=A0=A0=A0=A0=A0=A0=A0 httpClient.getParams().setConnectionM= anagerTimeout(30000);

remove this and ask users to must use serviceC= lient.cleanupTransport() since option 2 and 3 requires this. otherwise conn= ections are not released and the system hangs.

thanks,
Amila.

Jarek

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




--
Amila Suria= rachchi
WSO2 Inc.
blog: http://amilachinthaka.blogspot.com/
--0016364d2cbd7f1ba004908370df--