You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by bu...@apache.org on 2006/02/12 17:42:07 UTC
DO NOT REPLY [Bug 38624] New: - refactor HttpRequestExecutor and AsyncHttpProcessor
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
Summary: refactor HttpRequestExecutor and AsyncHttpProcessor
Product: HttpClient
Version: 4.0
Platform: Other
OS/Version: other
Status: NEW
Severity: normal
Priority: P2
Component: HttpCommon
AssignedTo: httpclient-dev@jakarta.apache.org
ReportedBy: http-async@dubioso.net
The class AsyncHttpProcessor duplicates most of the logic implemented
in HttpRequestExecutor. This was meant to be a temporary situation and
should be rectified.
two alternative patches coming up...
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-12 17:57 -------
Created an attachment (id=17667)
--> (http://issues.apache.org/bugzilla/attachment.cgi?id=17667&action=view)
alternative 2, take 1
Again, HttpRequestExecutor has the fine grained methods needed by the
asynchronous dispatchers. Here they are public to get rid of the
AsyncHttpProcessor altogether. To keep the HttpRequestExecutor clean,
the public fine-grained methods are confined to an inner class.
I like this version better, despite the public methods. An application
no longer has to create an HttpRequestExecutor and AsyncHttpProcessor
for synchronous and asynchronous requests. The dispatcher uses the
same HttpRequestExecutor as the application. This would allow for nice
coexistence of synchronous and asynchronous processing in a single
application.
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-12 17:51 -------
Created an attachment (id=17666)
--> (http://issues.apache.org/bugzilla/attachment.cgi?id=17666&action=view)
alternative 1, take 1
HttpRequestExecutor now has the fine-grained methods needed by
the asynchronous dispatchers. Those methods are protected, so as
to keep the mutable interfaces out of the public API. The
AsyncHttpProcessor is derived from the HttpRequestExecutor to
get access to the protected methods.
I also changed a line in the ElementalHttpServer, so I can use
the ElementalHttpServer to serve the ElementalHttpGet.
The patch causes one error when applied. To fix it, you have to edit
one line in AsyncHttpProcessor.java. Change the @version line to read:
* @version $Revision$ $Date$
The line is in the context of the patch for that class, and the
patch tool fails to match the context if svn has expanded the
magic tags. Of course this was also the root of previous problems
with my patches.
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
olegk@apache.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
Target Milestone|--- |4.0
------- Additional Comments From olegk@apache.org 2006-02-13 14:58 -------
Roland,
I clearly like the alternative 1 better. I would like to take it as a basic if
you can live with that
Oleg
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-25 17:42 -------
Created an attachment (id=17799)
--> (http://issues.apache.org/bugzilla/attachment.cgi?id=17799&action=view)
further patch, take 2
Here is another attempt to get rid of the AsyncHttpProcessor.
Compared to the previous patch, this one makes the doXxx methods
in HRX public (this time without an inner class). It also adds
JavaDocs regarding thread safety. Now that the mutable interfaces
are gone, making the methods public should be less problematic.
The method naming keeps potential user confusion at a minimum. The
logic previously found in AHP is now in the AbstractHttpDispatcher.
Please let me know what you think.
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-17 20:10 -------
Hi Oleg,
I think I got the drift of your changes. I like the removal of the
extra params from the SimpleDispatcher in favor of the params stored
anyway in the HttpRequestExecutor/AsyncHttpProcessor. You missed a
few @param params tags there, I get three javadoc warnings in async:
AbstractHttpDispatcher.java:136
AsyncHttpProcessor.java:90
SimpleHttpDispatcher.java:130
The renaming of the methods in HRX is fine by me. I intentionally
chose other terms than "send" and "receive" because there are
already too many ways of sending for my liking: with or without
preprocessing, sending on a connection or just queuing a request.
Anyway, I don't mind. You should remove the method name comment
I had put at the end of what is now receiveResponse.
I'm less comfortable with the send method having no flag to
disable the expect-continue handshake. The check you put into
AHP.transmitRequest makes assumptions about the implementation
of HRX.sendRequest by replicating the check for expect-continue.
It's not likely to cause a problem though, so I can live with it.
Could you update the JavaDoc of HRX.sendRequest to explicitly
state that the method will not attempt to receive anything unless
it has to handle an expect-continue handshake? That is an
important precondition for using it asynchronously, and it
should be documented there. I had it mentioned in the JavaDoc
for the parameter you removed.
Removing the 'terminal' flag from HRX.receiveResponse kills any
ideas I was pondering about how to make 1xx responses accessible
to applications that might need them. There's plenty of other
stuff to do that is more important, so I can live with that too.
For the time being :-)
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-26 10:00 -------
Hi Oleg,
there is no advantage from the http-core perspective. I expect
advantages from application developer perspective (for apps that
use http-async) and from http-client perspective. Both are in the
future. We can go with take 1 for the time being. That means
I'll have to change more code when this issue comes up again,
but so be it. Small steps. I remember :-)
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From olegk@apache.org 2006-02-13 22:18 -------
Cool. There are certian things in the patch, however, I personally would like to
see changed. I am working on take 2 based on alternative 1, which I would like
you to review once it is ready
Oleg
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From olegk@apache.org 2006-02-14 12:52 -------
Created an attachment (id=17686)
--> (http://issues.apache.org/bugzilla/attachment.cgi?id=17686&action=view)
Patch (take 2)
Roland, please review. I did go heavy-handed on your code. There were a few
things I did not quite like, mostly what I call code smell (abuse of method
parameters, modal methods, minor stuff like that). Please let me know if you
can live with these changes.
Oleg
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-13 21:44 -------
Hi Oleg,
yes, I can live with that. Once the mutable interfaces are gone
(see dev mailing list), we can discuss other options to get rid
of AsyncHttpProcessor completely. No hurry.
cheers,
Roland
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From olegk@apache.org 2006-02-17 23:00 -------
(In reply to comment #7)
Roland,
I checked in the patch. Please review the code once more and submit any
corrective patches you deem necessary. I am leaving the ticket open for the time
being.
My primary and foremost concern was that too many http-async concerns were
carried over into HttpRequestExecutor. Sometimes, code duplication is a lesser
evil than inheritance abuse and tight coupling of super and sub classes
>
> The renaming of the methods in HRX is fine by me. I intentionally
> chose other terms than "send" and "receive" because there are
> already too many ways of sending for my liking: with or without
> preprocessing, sending on a connection or just queuing a request.
> Anyway, I don't mind. You should remove the method name comment
> I had put at the end of what is now receiveResponse.
>
I do not mind calling those methods differently. I just did not like the fact
that some methods in sub class had a different parameter signature as methods in
the super class with the same name
>
> Removing the 'terminal' flag from HRX.receiveResponse kills any
> ideas I was pondering about how to make 1xx responses accessible
> to applications that might need them. There's plenty of other
> stuff to do that is more important, so I can live with that too.
> For the time being :-)
>
Any application that needs access to 1xx status codes should use HttpConnection
classes directly. HttpRequestExecutor cannot be everything to everyone
Oleg
PS: have you sent in your CLA?
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-25 17:36 -------
Created an attachment (id=17798)
--> (http://issues.apache.org/bugzilla/attachment.cgi?id=17798&action=view)
further patch, take 1
Hi Oleg,
here are some cleanup changes. Partly indentation and JavaDocs, but
also some method renaming and argument juggling. The methods in HRX
that implement steps of request processing are now all called doXxx.
Methods that have a different signature in AHP are called asyncXxx.
cheers,
Roland
one more patch coming up...
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
olegk@apache.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution| |FIXED
------- Additional Comments From olegk@apache.org 2006-02-26 14:59 -------
Further patch, take 1 checked in. Closing the bug for now
Oleg
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From olegk@apache.org 2006-02-25 22:35 -------
Roland,
I am not in favor of this change. What's the benefit for making all these
methods in HRX public from the HttpCore perspective? As far as I am concerned it
just makes HttpCore public API unnecessary complicated. Please keep AHP for the
time being.
Oleg
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
DO NOT REPLY [Bug 38624] - refactor HttpRequestExecutor and AsyncHttpProcessor
Posted by bu...@apache.org.
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG�
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=38624>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND�
INSERTED IN THE BUG DATABASE.
http://issues.apache.org/bugzilla/show_bug.cgi?id=38624
------- Additional Comments From http-async@dubioso.net 2006-02-20 07:15 -------
Hi Oleg,
sorry for the late reply, I was still suffering from a cold and did not
switch on the machine after Friday evening. I've slept over the problem
and I agree with your assessment of the 1xx problem. Both the send and
receive methods can be re-implemented outside the HRX/AHP if need be.
I'll probably have another look at the code this week-end.
cheers,
Roland
PS: It's in the mail :-)
--
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org