Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Sam Maloney wrote: 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). [ ... ] I would also agree. I would like and expect my use of the library to be one-to-one with respect to HTTP methods eg I ask it to do one thing and it does it. If anything makes the fetch not `complete' then it informs me and I decide whether to do a retry/follow a redirect/handle authentication. Also: - In my context the limits on redirects are not local to a call ie the number of redirects that are allowed on any one call may be affected by the general number of fetches that have been done so far. - I require full control of when a fetch is done. This is to allow limiting of number and type of hits to a web server. If auto-retry is enabled then I have no way of throttling or scheduling hits. As I think Laura has mentioned, it would be nice if HttpClient could be decomposed into reusable components for implementing a user agent, but it is essential that I be able to peel away any user agent layer that exists. From the sound of it, the current HttpMethodBase does too much on its own for me to use it. -- Mike - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Oleg Kalnichevski wrote: Mike, certain things in this life cannot be delayed. That includes patches for rapidly evolving software artifacts in my opinion ;-) Oleg I can understand that opinion. On the other hand I think that we should not loose application developers by changing interfaces/architecture too quickly/radically. With a branch you could start working on the new architecture tomorrow in theory. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Jandalf wrote: Odi's suggestion of starting a 2.1 branch now is a reasonable one. Just to make one thing clear. It is a 2.0 branch and not a 2.1 branch I am talking about: +-- 2.0 Final | ¦ CVS| ¦ merge bug fixes back | V ---X---X-X-- 2.1 (HEAD) 2.0 alpha 2 2.0 alpha 3 2.0 branch -X- designates a CVS tag It's a bad idea to start new features and new architecture on a branch. Always do this on the trunk. Why? Because there will be fewer and smaller back merges. Odi - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Jeff, no problem with either approaches. I would really welcome appearance of the 2.1 development branch. At the same time I can simply reproduce this patch after 2.0 release. Oleg On Wed, 2003-02-26 at 15:25, Jandalf wrote: Odi's suggestion of starting a 2.1 branch now is a reasonable one. Your patch is good Oleg, just a bit too tramatic for 2.0 right now. We are getting close to the point where we will want a 2.1 branch anyway. There would be a little extra overhead of maintaining the branch. Alternatively the patch will become stale and may be very difficult to apply later so that it would only be useful as a prototype. What do you say Oleg? BTW: The patch was never rejected. We all agree it is good work on the correct path. The problem is only when it makes sense to do this. It appears that I was wrong to target this as a 2.0 feature in the first place. Jandalf. Oleg Kalnichevski wrote: Hi Laura On Wed, 2003-02-26 at 00:24, Laura Werner wrote: Hi Oleg, Please remember those of us who aren't using the HttpClient class right now because it isn't quite flexible enough. If all of the redirect and authentication logic is moved there, I'd also like to see more flexibility, so that we don't have to write our own client classes that duplicate all of the new code that's going into HttpClient. 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 Regards 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 earlyfeedback
Hi all, 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. Agreed on all counts. I too really like this refactoring, as I mentioned earlier.. However as mentioned earlier this refactoring should not go into the code base until 2.0 has been released. I have mixed feelings on this one. Our company has a large, carrier-grade VoiceXML/telephony system. We're just about to deploy a version that incorporates the 2.0a1 version of httpclient. As soon as that's done (and I'm no longer getting dragged into random debug fests in other areas), I'm going to look at upgrading our code to use the latest httpclient, and to give our HttpCache a cleaner API so that we can think about open sourcing it at some point in the future. My (long-winded) point is that I'd rather do this in one big step than in multiple small ones. From my point of view, it's fine if the redirect work goes into 2.0. Of course, that's probably because 2.0 is too late to make it into my current release, so I don't care if it gets delayed a little bit. Someone who has a release coming up in a couple of months might think differently. -- Laura - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
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
Index: java/org/apache/commons/httpclient/HttpClient.java
===
RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpClient.java,v
retrieving revision 1.69
diff -u -r1.69 HttpClient.java
--- java/org/apache/commons/httpclient/HttpClient.java 21 Feb 2003 13:48:09 - 1.69
+++ java/org/apache/commons/httpclient/HttpClient.java 25 Feb 2003 19:49:47 -
-63,10 +63,14
package org.apache.commons.httpclient;
+import java.util.Set;
+import java.util.HashSet;
import java.io.IOException;
import java.net.URL;
-
+import java.net.MalformedURLException;
+import java.net.URL;
import org.apache.commons.httpclient.protocol.Protocol;
+import org.apache.commons.httpclient.util.URIUtil;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
-85,6 +89,7
* author a href=mailto:[EMAIL PROTECTED]Michael Becke/a
* author a href=mailto:[EMAIL PROTECTED]Mike Bowler/a
* author Sam Maloney
+ * author a href=mailto:[EMAIL PROTECTED]Oleg Kalnichevski/a
*
* version $Revision: 1.69 $ $Date: 2003/02/21 13:48:09 $
*/
-97,6 +102,9
/** Log object for this class. */
private static final Log LOG = LogFactory.getLog(HttpClient.class);
+/** Maximum number of redirects and authentications that will be followed */
+private static final int MAX_FORWARDS = 100;
+
// --- Constructors
/**
-149,6 +157,18
/** True if strict mode is enabled. */
private boolean strictMode = false;
+/** Whether or not I should automatically follow redirects. */
+private boolean followRedirects = true;
+
+/** Whether or not I should automatically processs authentication. */
+private boolean doAuthentication = true;
+
+/** Realms that we tried to authenticate to */
+private Set realms = null;
+
+/** Proxy Realms that we tried to authenticate to */
+private Set proxyRealms = null;
+
// - Properties
/**
-546,36 +566,73
}
}
-HttpConnection connection = state.getHttpConnectionManager().getConnection(
-methodConfiguration,
-httpConnectionTimeout
-);
-
-try {
-// Catch all possible exceptions to make sure to release the
-// connection, as although the user may call
-// Method-releaseConnection(), the method doesn't know about the
-// connection until HttpMethod.execute() is called.
-
-method.setStrictMode(strictMode);
+method.setStrictMode(strictMode);
-if (!connection.isOpen()) {
-connection.setSoTimeout(soTimeout);
-connection.setConnectionTimeout(connectionTimeout);
-connection.open();
-if (connection.isProxied() connection.isSecure()) {
-method = new ConnectMethod(method);
+this.realms = new HashSet();
+this.proxyRealms = new HashSet();
+//pre-emptively add
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Completely agree. A few months ago we have been discussing possibility of splitting HttpMethod into HttpRequest/HttpResponse pair once we get 2.0 is released. I would wait with more radical changes till then Cheers Oleg On Tue, 2003-02-25 at 21:38, Sam Maloney wrote: 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] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
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]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
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]
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Oleg, Great that you are taking on this work. I have both general and specific comments. General: * I share other's concerns with restructuring too much. At the risk of causing you much pain (since I'm not the one implementing!), how about the following: Create a new interface HttpMethodWithRedirect, defining getDoAuthentication(), setDoAuthentication(), execute(), getFollowRedirects(), and setFollowRedirects(), all of which are deprecated, and remove(!) the functions from HttpMethod. Have HttpMethodBase implement this new interface. Add a new function to HttpMethod, something like performRequestAndResponse(), which corresponds to your new model for the execute function. Old clients, using the existing method implementations that extend HttpMethodBase, could still get away with calling execute() on the method, although this would now be a deprecated use. HttpClient.executeMethod() would always invoke performRequestAndResponse(). My take is that the above changes, while theoretically breaking anything that implements HttpMethod without extending HttpMethodBase, I don't see as causing much of a problem, because likely nobody is successfully implementing HttpMethod outside of HttpMethodBase, seeing as we've had a lot of trouble getting it right. * Move the functionality that you've moved from HttpMethodBase to HttpClient to yet another new, package level class, perhaps with all static functions. Your new HttpClient and HttpMethodBase could call the shared implementation. * Seems like you need a function on HttpMethod called something like canFollowRedirects() that corresponds to what the unchanged EntityEnclosingMethod does with setFollowRedirects() - namely says you must not follow them. * HttpMethod.executeMethod() now suffers from the same problem that HttpMethodBase.execute() use to have. Some of the miscellaneous activity in this function ought to be extracted into separate routines. I suppose you can pass the buck on that one though. * I worry that your new changes are not going to handle proxies correctly, in weird cases with redirects. The execute method of ConnectMethod does an execute on its wrapped method. Not sure how your changes would match existing functionality, and off the top of my head, I cannot think of any obvious ways to restructure it. Specific: * HttpClient.getDefaultPort() - Protocol.getDefaultPort() would be appropriate to invoke here instead, I think. * HttpMethod.getFollowRedirects() - deprecate as well? This falls under my other comments. As for your question about connection usage, my perspective is that your changes are for the better. Right now, on a redirect, HttpMethodBase keeps the same connection (if it is persistent), and does the redirect or authentication. It seems to me that it would be a better model to return the connection to the pool and get a new one, if only for the architectural clarity of processing a single request/response pair (with a possible 100 Continue thrown in) with a connection, and then releasing the connection. Hope my comments help. -Elric. 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
Re: Significant HttpClient HttpMethodBase overhaul. Need earlyfeedback
Michael Becke wrote: 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. I implemented it this way in my HttpCache code that sits on top of httpclient but doesn't use the HttpClient class. There's an outer loop in my cache's executeMethod, and it calls the inner loop in HttpMethodBase.execute. It got messy and was a bit hard to get right, but it worked. In general, I think it's cleaner to have all of the redirection and authentication handled in one loop, rather than having two separate ones. It just makes the architecture simpler. 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. -- Laura - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
