On Fri, Sep 17, 2010 at 8:36 PM, Jarek Gawor <[email protected]> wrote:
> On Fri, Sep 17, 2010 at 7:55 AM, Amila Suriarachchi > <[email protected]> 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: [email protected] > For additional commands, e-mail: [email protected] > > -- Amila Suriarachchi WSO2 Inc. blog: http://amilachinthaka.blogspot.com/
