On Sun, 2005-12-11 at 15:24 +0100, Roland Weber wrote:
> Hi Oleg,
>
Hi Roland,
Please see my comments in-line
> I have taken a look at the HttpClientConnection and it's default
> implementation. You have implemented some protocol logic on the
> connection level which I would have expected to find in the method
> director. This design has serious drawbacks. Let me first explain
> the problems I see, then suggest an alternative interface.
>
>
> 1. HttpClientConnection.sendRequest will check for expect-continue
> handshake and wait for a 100-continue response if that is enabled.
> This clashes with asynchronous communication, in particular with
> pipelining. Several other requests may previously have been sent
> to the server, for which the responses have not yet been received.
> The connection will race with some other thread responsible for
> receiving responses to the preceeding requests.
>
Agreed.
>
> 2. HttpClientConnection.sendRequest will handle exactly one 1xx
> response if and only if the expect-continue handshake is enabled.
> RFC 2616, section 10.1 states:
>
> > A client MUST be prepared to accept one or more 1xx status responses
> > prior to a regular response, even if the client does not expect a 100
> > (Continue) status message.
>
> That results in inconsistent behavior: some 1xx responses are handled
> by the connection itself, others are returned to the caller. The logic
> to handle (or ignore) 1xx responses is spread across the code base.
>
I do not think this is the case. Only status codes 200 and above are
ever returned to the caller. See
DefaultHttpClientConnection#receiveResponse. I do have to admit that due
to the fact that the DefaultHttpClientConnection attempts to handle 1xx
status code internally it cannot be used for as a basis for implementing
HTTP/1.1 compliant proxies
>
> 3. HttpClientConnection.sendRequest will not give access to the
> first 1xx response if expect-continue handshake is enabled.
> RFC 2616, section 10.1 states:
>
> > Proxies MUST forward 1xx responses, unless the connection between the
> > proxy and its client has been closed, or unless the proxy itself
> > requested the generation of the 1xx response.
>
> Consider the following scenario for a proxy that uses HttpComponents
> to connect to the server. Note that this is *not* an academic scenario:
>
> - Client sends request with expect-continue handshake to the proxy.
> - Proxy MUST forward request with expect-continue handshake to server
> if server is HTTP/1.1 compliant (RFC 2616, section 8.2.3)
> - Server responds with 100-continue, waits for the request body.
> - Proxy MUST forward the 100-continue response to the client (see above).
> This is not possible, since the response is handled by the connection
> and never seen by the proxy code using the connection.
> - Proxy/connection can not forward the request body, since the client
> is still waiting for a 100-continue response before sending it.
> - Client doesn't receive a 100-continue response from proxy, remains
> stalled until some timeout hits (suggested in 8.2.3), then either
> aborts the request altogether or finally transmits the response body
> after an unnecessarily long delay.
>
>
Agreed.
> 4. (minor:) HttpClientConnection.receiveResponse requires the request
> as an argument, although the request has already been sent. This is
> necessary because receiveResponse has to decide whether the response
> has a body, which depends on the request method (HEAD vs. GET). Apart
> from that, only the parameters are accessed through the request. It
> is not intuitive that the request has to be passed again after it has
> been sent.
>
>
Agreed.
>
> Because of these problems, I suggest to completely remove the protocol
> logic from the HttpClientConnection interface. In particular, knowledge
> about the sequence of requests, responses, and entities, should be
> implemented exclusively on a layer above the connection. Also, sending
> and receiving should be completely decoupled, to allow for asynchronous
> communication. These are the methods I propose instead of the current
> sendRequest and receiveResponse:
>
> public interface HttpClientConnection {
>
> public void sendRequest(HttpRequest req, HttpParams params)
>
> public void sendEntity(HttpEntity entity, HttpParams params)
>
> public HttpResponse receiveResponse(HttpParams params)
>
> public HttpEntity receiveEntity(HttpParams params)
>
> } // interface HttpClientConnection
>
>
I think the idea to decouple request and entity processing logic as a
remedy for the problems identified above is a reasonable one. However, I
am not entirely sure it solves all the problems. At some point you will
have to assemble the complete HTTP response by combining the entity to
the header. However, since HttpResponse represents an immutable message
this will not be possible, unless you cast HttpResponse to
HttpMutableResponse, thus completely defeating the whole idea of
separating those two interfaces in the first place.
Les us try to redesign the HttpClientConnection interface, however, I do
suspect strongly the only clean solution to the problem may be having
different interfaces representing different types of connections: a
regular client connection, a proxy-side connection and a client
connection capable of pipelining requests at the expense of not
supporting 1xx status codes. I am afraid we may fail to come up with a
reasonably clean interface capable of representing all different types
of connections.
Oleg
>
> Passing 'params' to sendRequest() is not necessary since params are
> available from HttpRequest, but it is more consistent with the
> receive methods that need the params argument. sendRequest() sends
> only the request line, headers, and empty line, but no entity.
> sendEntity() could take an HttpEntityEnclosingRequest instead of the
> entity itself as an argument, though it shouldn't need anything but
> the entity and params.
> receiveResponse() receives only the status line and request headers.
> It doesn't need the request anymore, since it doesn't have to decide
> whether the response includes an entity. It returns all responses,
> including those with 1xx status codes. receiveEntity() could take
> an EntityGenerator as an argument, in that case sendEntity() should
> be changed to take an EntityWriter for the sake of symmetry.
> I have not dug deeper into the code, so I don't know how the data
> transmitter below the connection is affected by these suggestions.
>
>
> I'll be on a business trip for a few days, but I'm looking forward
> to reading your comments and responding when I'm back.
>
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> 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]
