Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
Cause with current CVS if an exception occurs reading response, the exception is going back to main thread, and even worse the connection isn't released (cause responseStream is null, so releaseConnection() does nothing), leaving connectionManager in a wrong state :( This should be fixed now. releaseConnection() now handles this case. Mike - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
RE: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
Sam Maloney a écrit :
>
> Makes sense to me,
> I would definatly agree on your point that 'Client' logic should be in
> HttpClient and not in HttpMethodBase. (I would say redirect, auth and
> even auto-retry would count as 'Client' logic).
>
> Sam
As for the auto-retry stuff, I agree it makes the HttpMethodBase really ugly
to read, I got lost several times trying to understand where my exception
camed from.
Wouldn't it be possible to clean the execute method (to remove extra stuff,
and keep it clear to read) and then add another method (in fact the method
is what Sam suggests to do when making requests in a app...) :
public int execute(HttpState state, HttpConnection conn, int maxRetries)
throws HttpException, IOException, NullPointerException
{
for(int i=0, i
Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
I do remember about it. In fact, in my initial post you have been personally mentioned to be among those impacted most by the suggested patch ;-) Had not the patch been turned down, I was going to approach you for suggestions on HttpClient improvement. I guess we will have to refactor HttpMethodBase HttpClient at a point of time after 2.0 release. We will definitely be in need of your feedback I would say delayed not turned down:) It seems that we should put some work in fixing up HttpClient to entice people to use it. This way when 2.x has made direct use of HttpMethods unwise people will have already moved to HttpClient. Perhaps we need a quick brainstorming session with all involved. Mike - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
A 2.0 branch sounds like a great idea to me. This way we can start messing around with post 2.0 development without dirtying the 2.0 release. Mike On Wednesday, February 26, 2003, at 03:23 AM, Ortwin Glück wrote: Oleg, I really like this refactoring. People like Laura should track the changes we make and rewrite their own client to either use or extend the HttpClient class. For the long run I think nobody should go without the HttpClient class. HttpClient should act a bit like a facade. However as mentioned earlier this refactoring should not go into the code base until 2.0 has been released. To make your life easier for you we could create a branch in CVS: - Create 2.0 Branch early (perhaps on 2.0 Beta): use this branch to check in bug fixes and release 2.0 - on the trunk: do the refactoring and merge back bug fixes from branch when appropriate. (Don't do it the other way round.) Jef, is that an option? Odi - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
To implement this feature (full redirects), there are really only three general choices that make sense: 1) augment HttpMethodBase.execute() Pros: current functionality is maintained Cons: makes complex code more complex, 2) have two stage redirection done partially in HttpMethodBase and part in HttpClien Pros: migration path for redirect logic to eventually be all in HttpClient, Cons: would be a hack prone to error 3) move and augment redirect logic into HttpClient.execute( Pros: most logical seperation of concerns, does simplify HttpMethodBase, Oleg has an initial patch Cons: breaks user code From the work that Oleg has done, and the comments here, it seems clear that redirect logic should be in HttpClient. Clearly Oleg is on the right track with his work, and I agree this is the best approach. The problem is that con is a pretty big drawback. I'd say it is to late in the development cycle to make such a change, no matter how rewarding. I did not realize how big of a deal true redirects was going to be to implement. Oleg's patch gives us a good base and evaluation platform, but perhaps we should consider putting this feature on the back burner and do it in 2.1. Jandalf. Adrian Sutton wrote: The only good reason I can see for going with a two-loop solution is the fact that we're fairly close to a final release right now and might want to keep the code stable. On the other hand, if we're going to make a semantic API change like this, it's probably better to do it now, before 2.0 final. Personally, I feel it's too late in the release process to be moving all the redirect code around, however that does depend on how inelegant the other options are. If we don't move it until after the 2.0 release, we should add a note on any method that is likely to be affected by the change that the change will happen and so you shouldn't depend on this code being here. (Much like Sun's warning about serializing Swing objects). I'd also suggest that we make HttpClient more flexible so that Laura can use it directly since that would then discourage people from using the methods directly and changes like this will be much less of a concern in the future. Course, I don't have time to actually do any of this, so feel free to just ignore me. :) Adrian Sutton, Software Engineer Ephox Corporation www.ephox.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
RE: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
>The only good reason I can see for going with a two-loop solution is the >fact that we're fairly close to a final release right now and might want >to keep the code stable. On the other hand, if we're going to make a >semantic API change like this, it's probably better to do it now, before >2.0 final. Personally, I feel it's too late in the release process to be moving all the redirect code around, however that does depend on how inelegant the other options are. If we don't move it until after the 2.0 release, we should add a note on any method that is likely to be affected by the change that the change will happen and so you shouldn't depend on this code being here. (Much like Sun's warning about serializing Swing objects). I'd also suggest that we make HttpClient more flexible so that Laura can use it directly since that would then discourage people from using the methods directly and changes like this will be much less of a concern in the future. Course, I don't have time to actually do any of this, so feel free to just ignore me. :) Adrian Sutton, Software Engineer Ephox Corporation www.ephox.com - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
I think it would be possible to add cross site redirects at the HttpClient level without removing the other functionality from the HttpMethod. HttpClient would just need to check the status code and re-try. But, just because it is possible doesn't mean we should do it. If we have the go-ahead to implement this I'm all for it. Perhaps we should try to make the redirection code available in some reusable format outside of HttpClient. This way the people who are not using HttpClient directly could still take advantage of it As far as compatibility with connection pooling goes there should be no problem. I have only gone over your patch at a high level, but it should work well. Mike On Tuesday, February 25, 2003, at 04:39 PM, Oleg Kalnichevski wrote: Mike I share your concerns, but the work on it has been blessed by Jandalf. Besides, I see no way of providing cross-site redirects while maintaining full compatibility with the existing HttpClient. Important question. Do you think that the patch will play well with your connection pooling stuff? Oleg On Tue, 2003-02-25 at 22:29, Michael Becke wrote: I definitely think this is a good idea. HttpMethodBase is too big. I'm wondering if this is too much of a change for 2.0 though. It will require quite a few changes for users. On a related note, when you implement the redirection handling I would suggest removing the use of the URL class. I think URI is better suited for this purpose. Also, using URL could be a problem with custom protocol types. Mike Oleg Kalnichevski wrote: Folks I am currently working on a patch enabling HttpClient to handle cross-site redirects. http://nagoya.apache.org/bugzilla/show_bug.cgi?id=16729 In order to lay foundation for this capability I needed to make quite a few changes to HttpClient & HttpMethodBase. I have opted for a more substantial overhaul of these classes, than was strictly necessary. I realize not all of you may agree with my decision. So, I decided to seek an early feedback from you to make sure I do not go completely astray. This is what I have done: I moved complete redirect & authenticate logic from HttpMethodBase to HttpClient. HttpMethodBase Impact: - Even though binary interface is unchanged, HttpClient's modus operandi with regard to redirect & authentication changed substantially. People like Laura Werner,who do not use standard HttpClient and have developed their own logic around lower level classes will be affected most. - Cleaner design. Redirect & authentication in my opinion logically do not belong to domain of the HTTP method, rather, they belong to that of the HTTP agent. - Over-convoluted HttpMethodBase class got simpler. Under-used HttpClient class is leveraged more. This is an important architectural improvement in my humble opinion. If you disagree, please let me know - I am seriously concerned that this redesign may have adversely affected connection pooling stuff. Mike, Eric, you are the connection pooling experts, could you please give me your opinion on that? - About a dozen of test cases have become obsolete. They will need to be redesigned. They are all commented out for the time being As always, any feedback, including that in a form of bad tomatoes thrown at me will be appreciated Please note, that cross-site redirect has not been implemented yet. Cheers Oleg - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient & HttpMethodBase overhaul. Need early feedback
Makes sense to me, I would definatly agree on your point that 'Client' logic should be in HttpClient and not in HttpMethodBase. (I would say redirect, auth and even auto-retry would count as 'Client' logic). Sam On Tuesday 25 February 2003 15:27, Oleg Kalnichevski wrote: > Folks > I am currently working on a patch enabling HttpClient to handle > cross-site redirects. > > http://nagoya.apache.org/bugzilla/show_bug.cgi?id=16729 > > In order to lay foundation for this capability I needed to make quite a > few changes to HttpClient & HttpMethodBase. I have opted for a more > substantial overhaul of these classes, than was strictly necessary. I > realize not all of you may agree with my decision. So, I decided to seek > an early feedback from you to make sure I do not go completely astray. > > This is what I have done: > > I moved complete redirect & authenticate logic from HttpMethodBase to > HttpClient. HttpMethodBase > > Impact: > > - Even though binary interface is unchanged, HttpClient's modus operandi > with regard to redirect & authentication changed substantially. People > like Laura Werner,who do not use standard HttpClient and have developed > their own logic around lower level classes will be affected most. > > - Cleaner design. Redirect & authentication in my opinion logically do > not belong to domain of the HTTP method, rather, they belong to that of > the HTTP agent. > > - Over-convoluted HttpMethodBase class got simpler. Under-used > HttpClient class is leveraged more. This is an important architectural > improvement in my humble opinion. If you disagree, please let me know > > - I am seriously concerned that this redesign may have adversely > affected connection pooling stuff. Mike, Eric, you are the connection > pooling experts, could you please give me your opinion on that? > > - About a dozen of test cases have become obsolete. They will need to be > redesigned. They are all commented out for the time being > > As always, any feedback, including that in a form of bad tomatoes thrown > at me will be appreciated > > Please note, that cross-site redirect has not been implemented yet. > > Cheers > > Oleg - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
