It looks good to me. A couple of minor things. - AsyncHttpClient.getCachedConnection() takes a SessionCache as an argument. I don't think this is necessary as the SessionCache is an instance variable of AsyncHttpClient? - We will need to reflect this API change to the Geronimo sample code that we have...
Thanks, Sangjin On 1/15/08, Rick McGuire <[EMAIL PROTECTED]> wrote: > > I opened a Jira on this issue: > > https://issues.apache.org/jira/browse/GERONIMO-3749 > > And attached a patch that I worked up to address this. This is > implemented by plugging a SessionCache instance into the AsyncHttpClient > instance. A couple notes about this: > > 1. I chose to use a set method instead of the a constructor argument > because the number of permutations of the constructors is already > a bit high. It might be better as a constructor argument, but I > don't think we want to try to add a permutations for all of the > possibilities. > 2. The reuseConnection attribute became redundant when this was > added. The cache is now used to trigger the reuse behavior. > > Please comment on the patch. I'll not commit anything until it appears > we have consensus. > > Rick > > Sangjin Lee wrote: > > Yes, if you used a different configuration for the SSL, then it would > > be another issue. > > > > The questions are: > > - Do we want to even allow an option for using a shared connection > cache? > > The benefit of a shared connection cache is ... just that; connection > > reuse will be maximized for the given process. However, the drawback > > is that you may run into unexpected socket configuration/SSL > > configuration behavior when you hand the connections to a different > > client instance. > > - If we do allow it, what should be the default? > > I think *not* sharing the connection cache might be a better default > > behavior. > > > > Thanks, > > Sangjin > > > > On 1/14/08, *Jarek Gawor* <[EMAIL PROTECTED] <mailto:[EMAIL PROTECTED]>> > > wrote: > > > > On 1/14/08, Sangjin Lee <[EMAIL PROTECTED] > > <mailto:[EMAIL PROTECTED]>> wrote: > > > It's a good point... Currently the session cache is global, and > all > > > AsyncHttpClient instances are forced to share it. The main > > thing that > > > concerns me is that as a result the connections will retain all > > the socket > > > properties that came from the AsyncHttpClient instance that > > opened the > > > connection. This might not be intuitive to say the least. For > > example, one > > > AsyncHttpClient instance opens a connection with TCP delay > > disabled, and > > > then another instance (this time with TCP delay enabled) comes > > along and > > > picks up this connection for reuse. Contrary to what it > > expects, it would > > > get a connection with TCP delay disabled. > > > > Right and this might be a bigger issue for SSL connections where you > > might need to distinguish between cached connections based on the > > client certs, CA certs, or enabled cipher suites (but I'm not sure > if > > these options can be set on the AsyncHttpClient). > > > > Jarek > > > > > > > >
