Hi Oleg,
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.
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.
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.
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.
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
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]
