hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Roland Weber <http-as...@dubioso.net>
Subject Re: [HttpConn] time for review
Date Sat, 13 Jan 2007 14:40:28 GMT
Hi Oleg,

> I like you have done so far and I think it is a good start. Now onto the
> details.

Thanks.

> My gripes are rather minor and mostly related to class / method naming. 

That is very important.

> (1) I actually think ManagedClientConnection would be a better name for
> the interface, because it reflects the fact that the connection is not
> self-containing and requires an external entity to manage its state. 

It needs an external entity, but that entity is the application and not
a connection manager. My thoughts about connection management so far
lead me to believe that the prepare/open/update methods in this form
should not be available at all for connections obtained from a connection
manager. There is no way to distinguish an update() for "tunnel created"
from an update() for "TLS/SSL socket layered".

> (2) Can we do without #prepare(Socket) method? Do we really need it?
> What is the point of giving the connection an unconnected socket?

I'm afraid not. I thought the JavaDocs were clear about the reason?

    This allows the connection to close that socket if shutdown
    is called before it is open. Closing the unconnected socket
    will interrupt a thread that is blocked on the connect.

See also https://issues.apache.org/jira/browse/HTTPCLIENT-475, the
original problem there was that connection.abort() does not close
the underlying socket on which a thread was blocked while connecting.
I'm open to better names than prepare(), but we need to tell the
connection which socket to close if it is aborted.

>> - org.apache.http.conn.SocketConnectionOperator
> 
> How about ClientConnectionOperaton?

Fine by me. I don't expect to see any non-socket operators in
HttpConn or HttpClient, so we can drop the Socket from the name.

>> a) Extended connection interface.
>> I can't develop the interface without rewriting the connection
>> manager to see what is actually needed. That's a major effort,
>> and I can't tell yet when I will have the time.
> 
> We need something that works _now_, something we can run the existing
> test cases against, before we get carried too far away with all sorts of
> fancy ideas. I feel very strongly that at this point of time having
> something that works should take precedence over API clarity. Let us
> just merge the functionality of the HttpHostConnection into the
> UnmanagedClientConnection, mark it 'to be reviewed', and keep changes to
> the existing connection managers to the minimum.

That's a fair request. My problem is the complexity of the MTHCM.
It's got several inner classes, including a wrapper for the connection.
And the original connection class has specific methods like tunnelCreated
which do not map to the new interface. As I wrote, it wasn't meant to be
for connections from a connection manager.
What needs to be done needs to be done, so I'll hack up a temporary
new interface for connection management. However, I'm not going to start
on this until I know that I've got more than just a few short hours on a
week-end. That thing is way too complex for a quick shot, in particular
since I'll have to rewrite the test cases too. It'll probably be February
until I find enough time, but I'll keep it at the top of my list.


>> b) Encapsulate tunnelling logic.
> 
> Let us just port the method director to HttpCore and then see what needs
> to be refactored and what could be improved.

There's no other way to go forward anyway. I just wanted to
put on record what I consider to be the long-term goal.

>> c) Determine socket security dynamically.
> 
> Give it a shot

Ok. Not this week-end though.

cheers,
  Roland


---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org


Mime
View raw message