+1 on this. I'm fine with the current behavior regarding HttpClient management. My only request would be to not instanciate a MultiThreadedHttpConnectionManager in the default case. A SimpleHttpConnectionManager would be good enough. I agree it's close to be a detail but still.
Alexis On Mon, Mar 23, 2009 at 9:20 PM, Amila Suriarachchi < amilasuriarach...@gmail.com> wrote: > > > On Tue, Mar 24, 2009 at 4:16 AM, Alexis Midon <mi...@intalio.com> wrote: > >> >> It's already possible to reuse the HttpClient and with your own >> HttpConnectionManager. What you have to do is create the HttpClient by >> yourself with your connection manager and push it in the MessageContext. > > > I think this is a good point. > So if we set both HTTPConstants.REUSE_HTTP_CLIENT, > HTTPConstants.CACHED_HTTP_CLIENT then ultimately transport sender use that > client. > This means users can manage the way they need to use the httpClient at > application level. either they can use one httpClient for all the messages > or kept the HttpState at thread local. > So IMHO since this is a performance fine tuning, it is ok to keep it in > this way and let users to mange it in the way they need at application > level. > > thanks, > Amila. > >> >> >> Alexis >> >> >> >> On Sun, Mar 22, 2009 at 3:52 AM, Amila Suriarachchi < >> amilasuriarach...@gmail.com> wrote: >> >>> hi all, >>> >>> recently I came across this link and according to this[1] creating new >>> http client for every request is not good >>> in performance wise. >>> >>> Current get Http client method looks like this >>> >>> 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); >>> } >>> } >>> >>> // Get the timeout values set in the runtime >>> initializeTimeouts(msgContext, httpClient); >>> return httpClient; >>> } >>> >>> According to the above article creating many http client instances are >>> not efficient. But if a user switch on the >>> HTTPConstants.REUSE_HTTP_CLIENT then he can not give an >>> MultiThreadedHttpConnectionManager object where users can define maximum >>> pool size, host configurations etc... >>> >>> Therefore I think it is better to have these features, >>> >>> 1. Let users to create the MultiThreadedHttpConnectionManager with its >>> own even when reusing http client. >>> 2. Does Http Client thread safe? if not we need to keep the cache copy of >>> the httpClient in a thread local variable. >>> >>> Does any one knows how to switch on the keep alive and will it make any >>> performance improvement? >>> >>> Anyway I still believe releasing connections should be done at the user >>> level since it degradates the performance. >>> >>> thanks, >>> Amila. >>> >>> >>> [1] http://hc.apache.org/httpclient-3.x/performance.html >>> >>> >>> On Tue, Mar 10, 2009 at 2:26 AM, Andreas Veithen < >>> andreas.veit...@gmail.com> wrote: >>> >>>> I agree that we also need to consider OperationClient and the >>>> non-blocking methods. I think that the first step towards a solid >>>> solution is actually to write a couple of test cases that provide >>>> evidence for these issues and that we can later use for regression >>>> testing, but I will have to review the existing unit tests we have. >>>> >>>> Andreas >>>> >>>> On Wed, Mar 4, 2009 at 01:21, Alexis Midon <mi...@intalio.com> wrote: >>>> > Another case where "Options#setCallTransportCleanup for >>>> OperationClient" is >>>> > obvious is when you call OperationClient#execute in a non-blocking >>>> way. >>>> > The caller cannot clean up the transport safely, because the execution >>>> might >>>> > still be in progress. In that case it's OperationClient responsability >>>> to >>>> > clean up the transport. >>>> > >>>> > Alexis >>>> > >>>> > >>>> > On Mon, Mar 2, 2009 at 10:11 AM, Alexis Midon <mi...@intalio.com> >>>> wrote: >>>> >> >>>> >> There is still some inconsistencies between how >>>> ServiceClient#sendReceive >>>> >> and Operation#execute use Options#getCallTransportCleanup. >>>> >> >>>> >> And I think that would help a lot if the related jira issues get >>>> cleaned >>>> >> up a little bit. >>>> >> >>>> >> Thanks for your time and feedback. >>>> >> >>>> >> Alexis >>>> >> >>>> >> >>>> >> On Sun, Mar 1, 2009 at 9:02 PM, Amila Suriarachchi >>>> >> <amilasuriarach...@gmail.com> wrote: >>>> >>> >>>> >>> hi all, >>>> >>> >>>> >>> On Fri, Feb 27, 2009 at 6:35 PM, Andreas Veithen >>>> >>> <andreas.veit...@gmail.com> wrote: >>>> >>>> >>>> >>>> I think that callTransportCleanup should never be turned on by >>>> default >>>> >>>> because it would disable deferred parsing of the response. What >>>> needs >>>> >>>> to be done urgently is to improve the documentation of the >>>> >>>> ServiceClient class to make it clear that it is mandatory to either >>>> >>>> call cleanupTransport explicitly or to set callTransportCleanup to >>>> >>>> true. Also the cleanupTransport method itself doesn't have any >>>> >>>> Javadoc. >>>> >>> >>>> >>> thanks for in-depth analysing of this issue. If the issue is not >>>> calling >>>> >>> to transport clean up then I clearly agree with what Andreas. >>>> >>> >>>> >>> Axis2 uses deffered building. There when the user gets the response >>>> >>> OMElement it has not build and Axis2 does not know when he going to >>>> finish >>>> >>> with it. So it is responsibility of the user to call the transport >>>> clean up >>>> >>> once they have done with the OMElement. >>>> >>> >>>> >>> But this problem does not occur with the generated data bind code. >>>> There >>>> >>> before returning from the stub method it generates the all the >>>> databinding >>>> >>> classes. In the generated code finally method calls >>>> >>> >>>> _messageContext.getTransportOut().getSender().cleanup(_messageContext); >>>> >>> >>>> >>> to clean up the transport. >>>> >>> >>>> >>> thanks, >>>> >>> Amila. >>>> >>>> >>>> >>>> Andreas >>>> >>>> >>>> >>>> On Fri, Feb 27, 2009 at 12:19, Dobri Kitipov >>>> >>>> <kdobrik.ax...@googlemail.com> wrote: >>>> >>>> > Hi Andreas, >>>> >>>> > thank you for the comment. I think you get the question. >>>> >>>> > Quick test shows that setting the following line of code into the >>>> >>>> > client: >>>> >>>> > >>>> >>>> > options.setCallTransportCleanup(true); >>>> >>>> > >>>> >>>> > forces the closure of the http connection. It seems it is not the >>>> >>>> > default >>>> >>>> > behavior. This is a good and fast solution. I was a little bit >>>> more >>>> >>>> > focused >>>> >>>> > on wondering why I have such a difference b/n using SOAP and MIME >>>> >>>> > builder. >>>> >>>> > >>>> >>>> > I need to think about some use cases when we need to have >>>> >>>> > options.setCallTransportCleanup(false). Can we have this by >>>> default in >>>> >>>> > some >>>> >>>> > cases? >>>> >>>> > >>>> >>>> > Anyway, it will be worth having a further analysis of the issue >>>> we >>>> >>>> > have with >>>> >>>> > SOAPBuilder behavior. >>>> >>>> > >>>> >>>> > Thank you, >>>> >>>> > Dobri >>>> >>>> > >>>> >>>> > >>>> >>>> > On Fri, Feb 27, 2009 at 12:46 PM, Andreas Veithen >>>> >>>> > <andreas.veit...@gmail.com> wrote: >>>> >>>> >> >>>> >>>> >> If I understand correctly, Dobri's findings can be summarized as >>>> >>>> >> follows: >>>> >>>> >> 1. Once the InputStream is consumed, commons-httpclient >>>> automatically >>>> >>>> >> releases the connection. >>>> >>>> >> 2. SOAPBuilder never completely consumes the InputStream. >>>> >>>> >> >>>> >>>> >> The SOAPBuilder behavior is indeed somewhat questionable, but it >>>> is >>>> >>>> >> important to understand that because of the deferred parsing >>>> model >>>> >>>> >> used by Axiom, there is never a guarantee that the InputStream >>>> will >>>> >>>> >> be >>>> >>>> >> completely consumed. Normally releasing the connection is the >>>> >>>> >> responsibility of the CommonsHTTPTransportSender#cleanup method >>>> which >>>> >>>> >> should be called by ServiceClient#cleanupTransport. It would be >>>> >>>> >> interesting to know if that method is called, and if it is, why >>>> it >>>> >>>> >> fails to release the connection. >>>> >>>> >> >>>> >>>> >> Andreas >>>> >>>> >> >>>> >>>> >> On Fri, Feb 27, 2009 at 10:10, Dobri Kitipov >>>> >>>> >> <kdobrik.ax...@googlemail.com> wrote: >>>> >>>> >> > Hi all, >>>> >>>> >> > I have observed absolutely the same thing these days. I need >>>> some >>>> >>>> >> > more >>>> >>>> >> > time >>>> >>>> >> > to analyze the whole picture, but here is my current synthesis >>>> of >>>> >>>> >> > the >>>> >>>> >> > issue. >>>> >>>> >> > >>>> >>>> >> > It seems that http connection release is tightly coupled with >>>> the >>>> >>>> >> > Axis2 >>>> >>>> >> > builder used to handle and process the response body and the >>>> >>>> >> > corresponding >>>> >>>> >> > input streams used. The builder and the InputStream used are >>>> based >>>> >>>> >> > on >>>> >>>> >> > the >>>> >>>> >> > HTTP headers fields like "Content-Type" and >>>> "Transfer-Encoding" >>>> >>>> >> > (e.g. >>>> >>>> >> > Content-Type: text/xml; charset=UTF-8 Transfer-Encoding: >>>> chunked). >>>> >>>> >> > So >>>> >>>> >> > if we >>>> >>>> >> > have Content-Type: text/xml; then SOAPBuilder class will be >>>> used. >>>> >>>> >> > If we >>>> >>>> >> > have type="application/xop+xml" then MIMEBuilder will be >>>> used. >>>> >>>> >> > >>>> >>>> >> > The successfull story when we have MIMIBuilder: >>>> >>>> >> > >>>> >>>> >> > When MIMEBuilder is used then the response Buffered >>>> InputStream >>>> >>>> >> > (IS) is >>>> >>>> >> > wrrapped (I will use "->" sign as substitution for wrrapped) >>>> >>>> >> > ChunkedIS >>>> >>>> >> > -> >>>> >>>> >> > AutoCloseIS (this has a responseConsumed() method notified >>>> when >>>> >>>> >> > IS.read() >>>> >>>> >> > returns -1, which means that the response IS has been >>>> completely >>>> >>>> >> > read) >>>> >>>> >> > -> >>>> >>>> >> > PushBackIS ->BounderyPushBackIS. The BounderyPushBackIS reads >>>> the >>>> >>>> >> > response >>>> >>>> >> > stream (see readFromStream(....)) in a cycle till it reaches >>>> its >>>> >>>> >> > end. At >>>> >>>> >> > every iteration of this cycle a AutoCloseIS checkClose(l) is >>>> >>>> >> > invoked. So >>>> >>>> >> > when the end is reached (-1 is returned) then this check >>>> causes the >>>> >>>> >> > invokation of the AutoCloseIS checkClose(...) method. This >>>> method >>>> >>>> >> > invokes >>>> >>>> >> > notifyWatcher() that in turn invokes responseBodyConsumed() >>>> method >>>> >>>> >> > of >>>> >>>> >> > the >>>> >>>> >> > HttpMethodBase class. This causes the release of the http >>>> >>>> >> > connection >>>> >>>> >> > which >>>> >>>> >> > is returned back to the connection pool. So here we have no >>>> problem >>>> >>>> >> > with >>>> >>>> >> > connection reuse. >>>> >>>> >> > >>>> >>>> >> > The bad story we have with SOAPBuilder: >>>> >>>> >> > >>>> >>>> >> > When SOAPBuilder is used then the response Buffered >>>> InputStream is >>>> >>>> >> > wrrapped >>>> >>>> >> > in a ChunkedIS -> AutoCloseIS -> PushBackIS. May be you has >>>> noticed >>>> >>>> >> > that >>>> >>>> >> > BounderyPushBackIS is not used. As a result the respose IS is >>>> not >>>> >>>> >> > completely >>>> >>>> >> > read (in fact this is not really correct, it could be read but >>>> the >>>> >>>> >> > invokation of the PushBackIS unread(...) method causes the >>>> >>>> >> > AutoCloseIS >>>> >>>> >> > checkClose() method never to return -1). As a result the http >>>> >>>> >> > connection >>>> >>>> >> > is >>>> >>>> >> > not released. And since there is a limit to have 2 connection >>>> per >>>> >>>> >> > host >>>> >>>> >> > then after the second invokation of the WS client the thread >>>> hangs >>>> >>>> >> > waiting >>>> >>>> >> > for a free connection. >>>> >>>> >> > >>>> >>>> >> > Please, provide us with your comments on this issue. >>>> >>>> >> > >>>> >>>> >> > Thank you in advance. >>>> >>>> >> > >>>> >>>> >> > Regards, >>>> >>>> >> > Dobri >>>> >>>> >> > >>>> >>>> >> > On Fri, Feb 27, 2009 at 3:54 AM, Alexis Midon < >>>> mi...@intalio.com> >>>> >>>> >> > wrote: >>>> >>>> >> >> >>>> >>>> >> >> no taker for an easy patch? >>>> >>>> >> >> >>>> >>>> >> >> Alexis >>>> >>>> >> >> >>>> >>>> >> >> >>>> >>>> >> >> On Wed, Feb 25, 2009 at 6:45 PM, Alexis Midon < >>>> mi...@intalio.com> >>>> >>>> >> >> wrote: >>>> >>>> >> >>> >>>> >>>> >> >>> Hi everyone, >>>> >>>> >> >>> >>>> >>>> >> >>> All the issues relatives to AXIS2-935 are really messy, some >>>> of >>>> >>>> >> >>> them >>>> >>>> >> >>> are >>>> >>>> >> >>> closed but their clones are not. Some are flagged as fixed >>>> but >>>> >>>> >> >>> are >>>> >>>> >> >>> obviously >>>> >>>> >> >>> not. All these issues are really old, so I'd like to take a >>>> >>>> >> >>> chance to >>>> >>>> >> >>> bring >>>> >>>> >> >>> them back to your attention, especially before releasing >>>> 1.5. >>>> >>>> >> >>> >>>> >>>> >> >>> I'll post a description of the issue in this email as a >>>> summary >>>> >>>> >> >>> all >>>> >>>> >> >>> the >>>> >>>> >> >>> jiras. >>>> >>>> >> >>> >>>> >>>> >> >>> By default, ServiceClient uses one HttpConnectionManager per >>>> >>>> >> >>> invocation >>>> >>>> >> >>> [2]. This connection manager will create and provide one >>>> >>>> >> >>> connection to >>>> >>>> >> >>> HTTPSender. The first issue is that by default this >>>> connection is >>>> >>>> >> >>> never >>>> >>>> >> >>> released to the pool [3]. if you do zillions of invocations, >>>> this >>>> >>>> >> >>> leak >>>> >>>> >> >>> will >>>> >>>> >> >>> max out your number of file descriptors. >>>> >>>> >> >>> >>>> >>>> >> >>> Your investigations in Axis2 options quickly lead you to the >>>> >>>> >> >>> REUSE_HTTP_CLIENT option. But this first issue has some >>>> >>>> >> >>> unfortunate >>>> >>>> >> >>> consequences if you activate it. Actually if you do so, a >>>> single >>>> >>>> >> >>> connection >>>> >>>> >> >>> manager is shared across all invocations. But because >>>> connections >>>> >>>> >> >>> are >>>> >>>> >> >>> not >>>> >>>> >> >>> release, the pool is starved after two invocations, and the >>>> third >>>> >>>> >> >>> invocation >>>> >>>> >> >>> hangs out indefinitely. :( >>>> >>>> >> >>> >>>> >>>> >> >>> If you keep digging you will find the >>>> AUTO_RELEASE_CONNECTION >>>> >>>> >> >>> option. >>>> >>>> >> >>> Its >>>> >>>> >> >>> sounds like a good lead! Let's try it. If you activate this >>>> >>>> >> >>> option the >>>> >>>> >> >>> connection is properly released -Yahoooo! the leak is fixed >>>> - but >>>> >>>> >> >>> unfortunately a new issue shows up (issue #2, aka >>>> AXIS2-3478). >>>> >>>> >> >>> AbstractHTTPSender passes the stream of the connection to >>>> the >>>> >>>> >> >>> message >>>> >>>> >> >>> context [4] , but that the connection is now properly >>>> released, >>>> >>>> >> >>> so >>>> >>>> >> >>> this >>>> >>>> >> >>> stream is closed before the SOAPBuilder gets a chance to >>>> read the >>>> >>>> >> >>> response >>>> >>>> >> >>> body. >>>> >>>> >> >>> Boom! "IOException: Attempted read on closed stream" >>>> >>>> >> >>> >>>> >>>> >> >>> These issues are easily reproducible in versions 1.3, 1.4, >>>> 1.5. >>>> >>>> >> >>> >>>> >>>> >> >>> I submitted and documented a fix in AXIS2-2931 [5], if you >>>> had a >>>> >>>> >> >>> chance >>>> >>>> >> >>> to look at it that would be much appreciate. >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> Alexis >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> [1] >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> https://issues.apache.org/jira/browse/AXIS2-935?focusedCommentId=12513543#action_12513543 >>>> >>>> >> >>> [2] see method getHttpClient in >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> https://svn.apache.org/repos/asf/webservices/commons/trunk/modules/transport/modules/http/src/org/apache/axis2/transport/http/AbstractHTTPSender.java >>>> >>>> >> >>> [3] see method cleanup in >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> https://svn.apache.org/repos/asf/webservices/commons/trunk/modules/transport/modules/http/src/org/apache/axis2/transport/http/HTTPSender.java >>>> >>>> >> >>> [4] see method processResponse in AbstractHTTPSender.java >>>> >>>> >> >>> [5] >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> >>>> >> >>> >>>> https://issues.apache.org/jira/browse/AXIS2-2931?focusedCommentId=12676837#action_12676837 >>>> >>>> >> >> >>>> >>>> >> > >>>> >>>> >> > >>>> >>>> > >>>> >>>> > >>>> >>> >>>> >>> >>>> >>> >>>> >>> -- >>>> >>> Amila Suriarachchi >>>> >>> WSO2 Inc. >>>> >>> blog: http://amilachinthaka.blogspot.com/ >>>> >> >>>> > >>>> > >>>> >>> >>> >>> >>> -- >>> Amila Suriarachchi >>> WSO2 Inc. >>> blog: http://amilachinthaka.blogspot.com/ >>> >> >> > > > -- > Amila Suriarachchi > WSO2 Inc. > blog: http://amilachinthaka.blogspot.com/ >