You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Roland Weber <ht...@dubioso.net> on 2007/01/05 20:17:20 UTC
[HttpConn] time for review
Hi all,
I've spent some time on reworking interfaces for HttpConn.
While the work is far from completed, some of the interfaces
are ready for review. As usual, I've spent more time than
I had planned, so I need to declare a break here in order to
take care of some other stuff for a while. I'll just give
you a summary of the changes and an outlook.
=== Changes ===
I didn't touch the old code moved over from HttpClient 3.x.
We have two new interfaces, with default implementations:
- org.apache.http.conn.UnmanagedClientConnection
A connection with the methods needed to open and tunnel.
It does not provide the data needed by a connection manager,
so it can not replace the old HttpHostConnection.
- org.apache.http.conn.SocketConnectionOperator
Encapsulates the knowledge about using SocketFactory
and SecureSocketFactory in order to open or update an
UnmanagedClientConnection. It does not know about
tunnelling.
This interface is meant to be thread safe. We could
also change the name to include a "Client" somewhere.
Since HttpConn is currently part of HttpClient, I
forgot that when I chose the name.
- Two examples show how to use the SocketConnectionOperator.
I learned quite a bit about tunnelling TLS/SSL connections
through proxies when coding the second example :-)
=== Outlook ===
a) Extended connection interface.
We do need another connection interface that provides
additional methods for connection management. The name I
have in mind is "ClientConnection", that's why the default
implementation of UnmanagedClientConnection is already
called DefaultClientConnection.
I have had some ideas, and some more ideas, and yet other
ideas about this interface. In the end, they are pointless.
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.
b) Encapsulate tunnelling logic.
The SocketConnectionOperator does only half of the job
when opening a secure connection through a proxy. It is
missing the tunnelling logic. Since tunnelling requires
generating HTTP requests, it is something that can not
simply be hidden in a socket factory or connection manager.
In HttpClient 3.x, the method director knows about
tunnelling. That class is rather inflexible and designed
exclusively for a blocking scenario. Reworking it is
another major effort. I'd like to have an interface where
you feed in a few parameters and which then tells what
to do next: create a tunnel, retry with authentication,
follow a redirect, and so on. This might be an occasion
to revisit HTTPCLIENT-63.
https://issues.apache.org/jira/browse/HTTPCLIENT-63
c) Determine socket security dynamically.
This point is somehow separate from connection interfaces,
but I stumbled across it again when implementing the default
socket connection operator. I'd like to have a method
SocketFactory.isSecure(Socket)
to query dynamically whether a socket should be considered
secure or not. I dislike instanceof checks to determine
object behavior. They tie runtime properties to a static
class hierarchy.
This change should not take more than a few hours to
implement. I'd like to get some feedback on the idea,
then I would tackle it on the next free weekend.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
SocketFactory.isSecure (was: [HttpConn] time for review)
Posted by Roland Weber <ht...@dubioso.net>.
Hi all,
Oleg Kalnichevski wrote:
> On Fri, 2007-01-05 at 20:17 +0100, Roland Weber wrote:
>
>> c) Determine socket security dynamically.
>
> Give it a shot
Done. I've stopped short of renaming SecureSocketFactory to
LayeredSocketFactory since the name is still appropriate in
most cases and I don't want to confuse users. However,
Scheme.isSecure() has changed to Scheme.isLayered().
The new connection classes are modified. The old classes are
still the same, relying on instanceof checks. It would have been
messy to hack into them, and I'll be working on replacements
anyway.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Roland Weber <ht...@dubioso.net>.
Whooa, mix-ups...
> See also https://issues.apache.org/jira/browse/HTTPCLIENT-475, the
> original problem there was that connection.abort() does not close
method.abort()
> 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.
shut down.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Michael Becke <mb...@gmail.com>.
> I got it. When being allocated, the connection was counted
> for the "connections per host" limit. When it is released,
> some other thread waiting for a connection to that host can
> be unblocked. We need to remember the host configuration
> for which the connection was allocated to uncount it when
> it gets released.
That is correct.
> I guess there is not even an unspecific list? Connections
> are kept in two lists simultaneously, one is always host
> specific while the other one keeps all connections in the
> pool. Currently, connections are never re-assigned to a
> different host, but instead they are deleted and a new
> connection is created for the new host.
It does keep a list of freeConnections (for the purpose of deleting a
least recently used connection). This only occurs when the host limit
_has not_ been reached, the total allocated connection limit _has_
been reached, and there are free connections. As you say though a
connection is never switched between hosts, it is just deleted and a
new one is created.
> I was a bit confused that HttpMethodDirector updates a
> HostConfiguration when following redirects, but that is
> not the HostConfiguration stored in the connection.
Yes, this is one of the strange "features" of the connection manager.
The current limits on connections per host (route really) are only
enforced at checkout time. Once a connection is outside of the
connection manager it can be reused for other hosts.
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Roland Weber <ht...@dubioso.net>.
> It seems that even closed connections are being put back
> into the pool for the host to which they were opened.
I got it. When being allocated, the connection was counted
for the "connections per host" limit. When it is released,
some other thread waiting for a connection to that host can
be unblocked. We need to remember the host configuration
for which the connection was allocated to uncount it when
it gets released.
> In my first attempt, I delete the HostConfiguration when
> the connection gets closed. The result is a SEVERE error
> message when the connection is released. I would have
> expected the closed or unassigned connections to just
> end up in the non-specific list of free connections.
I guess there is not even an unspecific list? Connections
are kept in two lists simultaneously, one is always host
specific while the other one keeps all connections in the
pool. Currently, connections are never re-assigned to a
different host, but instead they are deleted and a new
connection is created for the new host.
I was a bit confused that HttpMethodDirector updates a
HostConfiguration when following redirects, but that is
not the HostConfiguration stored in the connection.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Roland Weber <ht...@dubioso.net>.
Hi Mike,
looks like I was a bit premature: I do have a question :-)
It seems that even closed connections are being put back
into the pool for the host to which they were opened.
In my first attempt, I delete the HostConfiguration when
the connection gets closed. The result is a SEVERE error
message when the connection is released. I would have
expected the closed or unassigned connections to just
end up in the non-specific list of free connections.
Is my understanding correct, and would it be acceptable
to handle connections that way?
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Roland Weber <ht...@dubioso.net>.
Hi Mike,
> I'm sorry I didn't see your email earlier or I could have saved you
> some headaches. Sounds like you've figured it out though. Your
> description of the weak references, etc below is spot on and quite
> well written.
Thanks, it's good to learn that I figured it out correctly.
> Please let me know if there is anything I can add/explain.
For the time being, I'm reasonably comfortable with the code.
But I'd appreciate it very much if you could spend some time on
a code review once I'm through with the rewrite. Since you've got
the most experience with the MTHCM, you're in the best position
to spot where I might have gone into the wrong direction. Due to
the collection classes, there are plenty of downcasts. So I can't
rely on the compiler's type checks when dealing with the
replacements for the old connection interface on various levels.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Michael Becke <mb...@gmail.com>.
Hi Roland,
I'm sorry I didn't see your email earlier or I could have saved you
some headaches. Sounds like you've figured it out though. Your
description of the weak references, etc below is spot on and quite
well written. As you have discovered, most of the complexity of the
MTHCM comes from the support for handling GCed connections. This part
of the MTHCM was added after the original design was completed, so
there is likely a better/simpler way to achieve the same results.
Please let me know if there is anything I can add/explain.
Mike
On 1/20/07, Roland Weber <ht...@dubioso.net> wrote:
> Good news,
>
> some inspiring music on mind-shaking volume helped me
> solve the riddle of the MTHCM, and with a good night's
> sleep I'm ready to tackle the code. I even dimly remember
> the discussions about the garbage collection thing.
>
> The reason for the weak references is that an application
> may allocate a connection and then just forget about it
> without releasing it. Since the manager does not maintain
> a hard reference to the connection, it will be GCed and
> the vanishing weak reference tells the manager(s) that a
> connection has been lost. Without the connection object,
> it's hard to tell _which_ manager issued the connection
> that was lost, hence there is a second map from the weak
> references to the responsible managers.
> I guess this strategy was introduced before the wrappers
> that are being handed out nowadays. With the wrappers in
> place, we can manage the connections in the traditional
> way and instead keep a weak reference to the _wrapper_.
> When the wrapper is GCed, the connection can be reclaimed.
> That way, connections will never be cleaned up by the GC
> (except when the whole connection manager is GCed) and
> we don't have to rely on finalizers being called in order
> to release for example native resources associated with
> the connection.
>
> Another missing puzzle piece is the route tracking that
> is implemented in the old connection class by means of
> a HostConfiguration and tunnelCreated flag. The new
> interfaces have been defined bottom-up, so they don't
> have this feature which is required by the manager. It's
> not possible to map a managed connection to an operated
> connection by a simple adapter without adding route
> tracking. I'll need an extra class here, and that is the
> place where I can fit the subclassing I mentioned earlier.
>
> I'll get started right away...
>
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Roland Weber <ht...@dubioso.net>.
Good news,
some inspiring music on mind-shaking volume helped me
solve the riddle of the MTHCM, and with a good night's
sleep I'm ready to tackle the code. I even dimly remember
the discussions about the garbage collection thing.
The reason for the weak references is that an application
may allocate a connection and then just forget about it
without releasing it. Since the manager does not maintain
a hard reference to the connection, it will be GCed and
the vanishing weak reference tells the manager(s) that a
connection has been lost. Without the connection object,
it's hard to tell _which_ manager issued the connection
that was lost, hence there is a second map from the weak
references to the responsible managers.
I guess this strategy was introduced before the wrappers
that are being handed out nowadays. With the wrappers in
place, we can manage the connections in the traditional
way and instead keep a weak reference to the _wrapper_.
When the wrapper is GCed, the connection can be reclaimed.
That way, connections will never be cleaned up by the GC
(except when the whole connection manager is GCed) and
we don't have to rely on finalizers being called in order
to release for example native resources associated with
the connection.
Another missing puzzle piece is the route tracking that
is implemented in the old connection class by means of
a HostConfiguration and tunnelCreated flag. The new
interfaces have been defined bottom-up, so they don't
have this feature which is required by the manager. It's
not possible to map a managed connection to an operated
connection by a simple adapter without adding route
tracking. I'll need an extra class here, and that is the
place where I can fit the subclassing I mentioned earlier.
I'll get started right away...
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager
Posted by Roland Weber <ht...@dubioso.net>.
Hi Oleg,
> Mike put a lot of hard work into the multithreaded connection manager.
> To this point it still constitutes the most significant and complex
> contribution to the project. Even though it does look like some kind of
> black magic MTHCM has been working really well for us. I urge you to not
> start taking it apart until we have enough test coverage to test for
> regressions. Let's just get it working with the new API and then see
> what could be improved.
I appreciate the work that Mike and others have put into the MTHCM.
And I can well imagine how it developed over time. I'm sure that
originally connections were handed out directly, which explains why
they get deleted and created instead of being managed. Then the
wrappers came, and so on.
But I'm currently looking at the result of all these changes over
time. And I can't see where to start with making _small_ changes.
I can't "just make it work with the new API".
I thought it couldn't be so hard to have one interface for the
connections being managed and another one for the wrappers being
given to the caller. Then I look at the code and realize that the
connections are not being managed by reference, they are subclassed.
And the only purpose of the subclass is to create a weak reference
to the object itself, which will then be put into some static map,
to be retrieved I don't know when for I don't know what. Oh, and
there is code to put references in and out of those maps for reasons
I don't understand either.
The idea of moving some of the classes out to make things simpler
was a dead end too. The stuff is nested, with most of the complexity
being in the ConnectionPool class with plenty of private methods.
It won't help to move it to an extra file and make the methods public.
Basically nothing in MTHCM works in a way that is remotely in line
with my understanding of what the class is supposed to do. I don't
work or think that way. To me, a pool is a finite set of elements,
some of which are allocated and some of which are not. You keep
references to those elements and when the pool is shut down, you
shut down the elements. The manager *knows* which elements it is
managing. Weak references and garbage collection???
I'm sorry, but I have serious issues with this code. I'm not just
feeling a bit uncomfortable about it, and it's not "I would have
implemented this differently" either. It's not the formatting and
not the variable or method names. The class is based on an extremely
complex design which is completely alien to me. I believe that much
of the complexity will simply vanish if a more straightforward
design is applied to the problem.
I'll give the code one more try tomorrow. But only one.
Meanwhile, I'll appreciate any background information on the
current design that somebody can give me.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] manager (was: time for review)
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sat, 2007-01-20 at 22:26 +0100, Roland Weber wrote:
> Roland Weber wrote:
>
> > I'll try to get some work done on the connection manager this weekend.
>
> I can already tell that I won't have much to show this weekend.
> The integration of the connection manager with the garbage collector
> by means of some static maps and weak references is a nightmare to
> understand for a newcomer. I don't even know whether I should spend
> the time to understand it in detail or whether I should just rip it
> out, implement something that manages connections using the current
> algorithm, and think about the GC stuff afterwards.
> I am _very_ suspicious of those global maps. They prevent completely
> independent instances, and that smells like non-scalability. Some
> of the shutdown code relies on those maps, too. Looks like the
> manager does not explicitly track the connections it hands out and
> rather scans a global map of all connections to find those for which
> it is responsible. I cannot even tell how wrong this feels to me.
> But _maybe_ I just read the code wrong.
>
Roland,
Mike put a lot of hard work into the multithreaded connection manager.
To this point it still constitutes the most significant and complex
contribution to the project. Even though it does look like some kind of
black magic MTHCM has been working really well for us. I urge you to not
start taking it apart until we have enough test coverage to test for
regressions. Let's just get it working with the new API and then see
what could be improved.
Oleg
> cheers,
> Roland
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
[HttpConn] manager (was: time for review)
Posted by Roland Weber <ht...@dubioso.net>.
Roland Weber wrote:
> I'll try to get some work done on the connection manager this weekend.
I can already tell that I won't have much to show this weekend.
The integration of the connection manager with the garbage collector
by means of some static maps and weak references is a nightmare to
understand for a newcomer. I don't even know whether I should spend
the time to understand it in detail or whether I should just rip it
out, implement something that manages connections using the current
algorithm, and think about the GC stuff afterwards.
I am _very_ suspicious of those global maps. They prevent completely
independent instances, and that smells like non-scalability. Some
of the shutdown code relies on those maps, too. Looks like the
manager does not explicitly track the connections it hands out and
rather scans a global map of all connections to find those for which
it is responsible. I cannot even tell how wrong this feels to me.
But _maybe_ I just read the code wrong.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Roland Weber <ht...@dubioso.net>.
Hi Oleg,
> I played around with various options and found none of them satisfactory
> enough. I still think we may be able to do away with #prepare() method.
> There will be no need to "announce" an unconnected socket the connection
> at all we somehow manage to provide a mechanism to look up the socket by
> the HTTP request. This way we could abort request's execution by simply
> closing the socket. No need to muck around with the connection.
>
> Let's move onto other issues and deal with this one later
Ok. I'll try to get some work done on the connection manager this weekend.
Not sure how much time I'll find though - the Snooker Masters is on TV :-)
I'll focus on the implementation part, test cases will follow once we
have an understanding of how the API will shape up. Here's a run-down of
my ideas...
- I need to spin off some of the inner classes. There's just too many
of them for my liking. Since it'll be in an implementation package,
a few extra classes shouldn't matter.
- The old implementation uses connection wrappers, where both the
wrapper and the wrapped connection have the same API. I'll keep
the wrapper concept, but will introduce a different interface
for the wrapped one.
- Rename Unmanaged*Connection to Operated*Connection. That's the one
that will be wrapped. Managed*Connection would be the interface
exposed by the wrapper.
- HostConfiguration in it's current form is a modifiable implementation
of the "routes" I've been discussing in the Wiki. I think the name
is a misnomer, but I'll keep it for now in order to get something
working a.s.a.p.
- I'll put the methods to build a route (tunnelCreated and such) in
the connection wrapper and interface. That's where they are now,
and it allows for self-managed connections. In other words, classes
can handle routing without caring about the connection manager.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sun, 2007-01-14 at 22:25 +0100, Roland Weber wrote:
> Oleg Kalnichevski wrote:
> > On Sun, 2007-01-14 at 20:58 +0100, Roland Weber wrote:
> >> Oleg Kalnichevski wrote:
> >>
> >>> True. But I somehow I can't help thinking #open() and #update() might
> >>> suffice. Why not use #open() / #update() instead of #prepare / #open()?
> >> Ah, I get the idea. Modal behavior of open().
> >> I didn't think of that because it means calling
> >> open() with a socket that is not open :-)
> >
> > This is the reason why I personally prefer 'bind'/'rebind' semantics to
> > 'open/'update'.
>
> Make sure to change the method names accordingly in your patch.
> There's HttpConnection.isOpen(), which wouldn't match with an
> open() method that does not actually open the connection.
> But I'm afraid we'd need an isBound() method to determine whether
> a call to update/rebind is permissible, which would nullify the
> advantage of loosing the prepare() method.
>
> Btw, I think "announce" would be a better name than "prepare".
> It avoids the false assumption that preparation is mandatory.
>
Roland,
I played around with various options and found none of them satisfactory
enough. I still think we may be able to do away with #prepare() method.
There will be no need to "announce" an unconnected socket the connection
at all we somehow manage to provide a mechanism to look up the socket by
the HTTP request. This way we could abort request's execution by simply
closing the socket. No need to muck around with the connection.
Let's move onto other issues and deal with this one later
Oleg
> cheers,
> Roland
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Roland Weber <ht...@dubioso.net>.
Oleg Kalnichevski wrote:
> On Sun, 2007-01-14 at 20:58 +0100, Roland Weber wrote:
>> Oleg Kalnichevski wrote:
>>
>>> True. But I somehow I can't help thinking #open() and #update() might
>>> suffice. Why not use #open() / #update() instead of #prepare / #open()?
>> Ah, I get the idea. Modal behavior of open().
>> I didn't think of that because it means calling
>> open() with a socket that is not open :-)
>
> This is the reason why I personally prefer 'bind'/'rebind' semantics to
> 'open/'update'.
Make sure to change the method names accordingly in your patch.
There's HttpConnection.isOpen(), which wouldn't match with an
open() method that does not actually open the connection.
But I'm afraid we'd need an isBound() method to determine whether
a call to update/rebind is permissible, which would nullify the
advantage of loosing the prepare() method.
Btw, I think "announce" would be a better name than "prepare".
It avoids the false assumption that preparation is mandatory.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sun, 2007-01-14 at 20:58 +0100, Roland Weber wrote:
> Oleg Kalnichevski wrote:
>
> > True. But I somehow I can't help thinking #open() and #update() might
> > suffice. Why not use #open() / #update() instead of #prepare / #open()?
>
> Ah, I get the idea. Modal behavior of open().
> I didn't think of that because it means calling
> open() with a socket that is not open :-)
> >
This is the reason why I personally prefer 'bind'/'rebind' semantics to
'open/'update'.
Oleg
> > I'll try to put together a patch for your consideration.
>
> Thanks, I'll have a look at it next week-end.
>
> cheers,
> Roland
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Roland Weber <ht...@dubioso.net>.
Oleg Kalnichevski wrote:
> True. But I somehow I can't help thinking #open() and #update() might
> suffice. Why not use #open() / #update() instead of #prepare / #open()?
Ah, I get the idea. Modal behavior of open().
I didn't think of that because it means calling
open() with a socket that is not open :-)
>
> I'll try to put together a patch for your consideration.
Thanks, I'll have a look at it next week-end.
cheers,
Roland
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sat, 2007-01-13 at 15:40 +0100, Roland Weber wrote:
> 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.
>
True. But I somehow I can't help thinking #open() and #update() might
suffice. Why not use #open() / #update() instead of #prepare / #open()?
I'll try to put together a patch for your consideration.
Oleg
> >> - 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
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
Re: [HttpConn] time for review
Posted by Roland Weber <ht...@dubioso.net>.
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
Re: [HttpConn] time for review
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Fri, 2007-01-05 at 20:17 +0100, Roland Weber wrote:
> Hi all,
>
> I've spent some time on reworking interfaces for HttpConn.
> While the work is far from completed, some of the interfaces
> are ready for review. As usual, I've spent more time than
> I had planned, so I need to declare a break here in order to
> take care of some other stuff for a while. I'll just give
> you a summary of the changes and an outlook.
>
Hi Roland
I took the first (and rather cursory) look at the changes in HttpConn.
Here is my feedback so far. More to come at a later point.
I like you have done so far and I think it is a good start. Now onto the
details.
My gripes are rather minor and mostly related to class / method naming.
> === Changes ===
>
> I didn't touch the old code moved over from HttpClient 3.x.
> We have two new interfaces, with default implementations:
>
> - org.apache.http.conn.UnmanagedClientConnection
> A connection with the methods needed to open and tunnel.
> It does not provide the data needed by a connection manager,
> so it can not replace the old HttpHostConnection.
>
(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.
(2) Can we do without #prepare(Socket) method? Do we really need it?
What is the point of giving the connection an unconnected socket?
> - org.apache.http.conn.SocketConnectionOperator
> Encapsulates the knowledge about using SocketFactory
> and SecureSocketFactory in order to open or update an
> UnmanagedClientConnection. It does not know about
> tunnelling.
> This interface is meant to be thread safe. We could
> also change the name to include a "Client" somewhere.
> Since HttpConn is currently part of HttpClient, I
> forgot that when I chose the name.
How about ClientConnectionOperaton?
> - Two examples show how to use the SocketConnectionOperator.
> I learned quite a bit about tunnelling TLS/SSL connections
> through proxies when coding the second example :-)
>
> === Outlook ===
>
> a) Extended connection interface.
> We do need another connection interface that provides
> additional methods for connection management. The name I
> have in mind is "ClientConnection", that's why the default
> implementation of UnmanagedClientConnection is already
> called DefaultClientConnection.
> I have had some ideas, and some more ideas, and yet other
> ideas about this interface. In the end, they are pointless.
> 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.
> b) Encapsulate tunnelling logic.
> The SocketConnectionOperator does only half of the job
> when opening a secure connection through a proxy. It is
> missing the tunnelling logic. Since tunnelling requires
> generating HTTP requests, it is something that can not
> simply be hidden in a socket factory or connection manager.
> In HttpClient 3.x, the method director knows about
> tunnelling. That class is rather inflexible and designed
> exclusively for a blocking scenario. Reworking it is
> another major effort. I'd like to have an interface where
> you feed in a few parameters and which then tells what
> to do next: create a tunnel, retry with authentication,
> follow a redirect, and so on. This might be an occasion
> to revisit HTTPCLIENT-63.
> https://issues.apache.org/jira/browse/HTTPCLIENT-63
>
Let us just port the method director to HttpCore and then see what needs
to be refactored and what could be improved.
> c) Determine socket security dynamically.
> This point is somehow separate from connection interfaces,
> but I stumbled across it again when implementing the default
> socket connection operator. I'd like to have a method
> SocketFactory.isSecure(Socket)
> to query dynamically whether a socket should be considered
> secure or not. I dislike instanceof checks to determine
> object behavior. They tie runtime properties to a static
> class hierarchy.
> This change should not take more than a few hours to
> implement. I'd like to get some feedback on the idea,
> then I would tackle it on the next free weekend.
>
Give it a shot
Oleg
>
> cheers,
> Roland
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpcomponents-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpcomponents-dev-help@jakarta.apache.org