Intended use case of ChannelPool
891 views
Skip to first unread message
Tim Boudreau
Jun 9, 2015, 9:57:15 PM6/9/15
to ne...@googlegroups.com
I'd been waiting for ChannelPool to appear in Netty, to update my Netty HTTP Client[1] with a connection pool (per-host/port collection of open channels, so keep-alive connections can be reused).
So I finally spent a few hours working on that, but the design of ChannelPool does not make it easy. So I'm wondering if this use case just isn't the kind of thing it was meant for (it looks like it was intended for things like JDBC-style connection pools - a bunch of connections to one host, period).
Specific issues I'm running into:
- SimpleChannelPool calls the no-argument bootstrap.connect(). Easy enough to have a subclass that knows the host+port and have one instance per host+port. However, FixedChannelPool is a final subclass of SimpleChannelPool, so the only way to deal with that is to copy the source code of FixedChannelPool. Better would be to unfinal FixedChannelPool and final most of its methods; better still, design-wise, would be to factor the code that establishes the connection into a separate class.
- SimpleChannelPool calls bootstrap.handler(), clobbering whatever handler was previously set up on it (i.e. one that does something useful!) - replacing it with one that simply calls ChannelPoolHandler on various events (I'm not sure what ChannelPoolHandler is good for - logging?). This is part of a larger issue that SimpleChannelPool assumes it has the Bootstrap to itself, and that is not necessarily true.
- ChannelPool.release() is clunky - a cleaner pattern would be for a channel pool to wrap the real Channel object in one that delegates to it, and simply uses calls to close() to release the channel back to the pool. That way you don't have to go find every call that closes a channel in your codebase, give it a reference to the ChannelPool and replace calls to close with pool.release(channel) - which means that a whole lot of code has to be ChannelPool aware that probably shouldn't.
- Why return Future<Channel> from ChannelPool.acquire(), instead of ChannelFuture (which is a Future<Void> and therefore cannot be returned by it)? If you're using any of the connect() methods on Bootstrap, you're *going* to get a ChannelFuture, and this just forces everybody to write a bunch of boilerplate to create a promise, listen on the ChannelFuture and update the promise on completion.
At this point, I'm considering just writing connection pooling from scratch without this stuff, but if I'm using it wrong and it is intended for this sort of use case, maybe there are some changes that could be made to make the API a little more friendly to it.
-Tim
Tim Boudreau
Jun 9, 2015, 9:57:44 PM6/9/15
to ne...@googlegroups.com
이희승 (Trustin Lee)
Jun 9, 2015, 10:27:12 PM6/9/15
to ne...@googlegroups.com
Hi Tim,
Thank you very much for your feed back first of all.
On Wed, Jun 10, 2015, at 10:57 AM, Tim Boudreau wrote:
I'd been waiting for ChannelPool to appear in Netty, to update my Netty HTTP Client[1] with a connection pool (per-host/port collection of open channels, so keep-alive connections can be reused).So I finally spent a few hours working on that, but the design of ChannelPool does not make it easy. So I'm wondering if this use case just isn't the kind of thing it was meant for (it looks like it was intended for things like JDBC-style connection pools - a bunch of connections to one host, period).Specific issues I'm running into:- SimpleChannelPool calls the no-argument bootstrap.connect(). Easy enough to have a subclass that knows the host+port and have one instance per host+port. However, FixedChannelPool is a final subclass of SimpleChannelPool, so the only way to deal with that is to copy the source code of FixedChannelPool. Better would be to unfinal FixedChannelPool and final most of its methods; better still, design-wise, would be to factor the code that establishes the connection into a separate class.
Sounds like a good idea. We could probably introduce an interface that is used to establish a new connection and its default implementation that uses Bootstrap.
- SimpleChannelPool calls bootstrap.handler(), clobbering whatever handler was previously set up on it (i.e. one that does something useful!) - replacing it with one that simply calls ChannelPoolHandler on various events (I'm not sure what ChannelPoolHandler is good for - logging?). This is part of a larger issue that SimpleChannelPool assumes it has the Bootstrap to itself, and that is not necessarily true.
Introducing a new type dedicated for the establishment of a new connection (your suggestion in the first bullet point) will probably solve this issue together.
- ChannelPool.release() is clunky - a cleaner pattern would be for a channel pool to wrap the real Channel object in one that delegates to it, and simply uses calls to close() to release the channel back to the pool. That way you don't have to go find every call that closes a channel in your codebase, give it a reference to the ChannelPool and replace calls to close with pool.release(channel) - which means that a whole lot of code has to be ChannelPool aware that probably shouldn't.
Norman and I discussed about this while designing ChannelPool and we decided not to override the close() operation. It's mainly because we don't really know what handles the pipeline contains and it is likely to introduce some conflicts.
- Why return Future<Channel> from ChannelPool.acquire(), instead of ChannelFuture (which is a Future<Void> and therefore cannot be returned by it)? If you're using any of the connect() methods on Bootstrap, you're *going* to get a ChannelFuture, and this just forces everybody to write a bunch of boilerplate to create a promise, listen on the ChannelFuture and update the promise on completion.
Returning a ChannelFuture instead of Future<Channel> sounds like a good idea to me.
At this point, I'm considering just writing connection pooling from scratch without this stuff, but if I'm using it wrong and it is intended for this sort of use case, maybe there are some changes that could be made to make the API a little more friendly to it.
Thanks again for your feed back. I think you have some valid points and we should address them.
Cheers,
T
--
Tim Boudreau
Jun 9, 2015, 10:56:47 PM6/9/15
to ne...@googlegroups.com
- ChannelPool.release() is clunky - a cleaner pattern would be for a channel pool to wrap the real Channel object in one that delegates to it, and simply uses calls to close() to release the channel back to the pool. That way you don't have to go find every call that closes a channel in your codebase, give it a reference to the ChannelPool and replace calls to close with pool.release(channel) - which means that a whole lot of code has to be ChannelPool aware that probably shouldn't.Norman and I discussed about this while designing ChannelPool and we decided not to override the close() operation. It's mainly because we don't really know what handles the pipeline contains and it is likely to introduce some conflicts.
Okay, I can see that - if a connection is in a bad state and you really want to close it, there needs to be some way to do that, not return it to the pool.
I think the solution would be:
A separate ChannelPool implementation that does use close() to return the channel to the pool, which you can wrap around any other ChannelPool implementation if you want that behavior. And add an interface that wrapped channels will implement:
interface DelegatingChannel extends Channel {
Channel getDelegate();
}
Code that needs to close a channel "with prejudice" can do
if (channel instanceof DelegatingChannel) ((DelegatingChannel)channel).close();
which will remove it from the pool. That's not 100% transparent, but better than having everything be pool-aware (I wound up writing a non-pooling ChannelPool implementation to solve that, but it's far less code changes if close() just returns the Channel to the pool for most cases). Or maybe such code could just use disconnect() for that purpose?
What sorts of conflicts were you worried about?
-Tim
이희승 (Trustin Lee)
Jun 9, 2015, 11:14:44 PM6/9/15
to ne...@googlegroups.com
Some handlers intercept close() or disconnect(). SslHandler is a good example. Your idea of wrapping a channel with a delegating channel was also discussed, but we thought it didn't seem to help a lot. A user sometimes want to downcast a channel into a transport-specific one like EpollSocketChannel, and wrapping a channel with a delegator only makes things complicated. Perhaps we could add a method like 'isPooled(Channel)' to 'ChannelPool' instead?
if (pool.isPooled(channel)) { pool.release(channel); }
Tim Boudreau
Jun 10, 2015, 2:16:41 AM6/10/15
to ne...@googlegroups.com
Maybe take a similar approach to what JDBC's Wrapper does, so if you want a specific type, you don't have to cast - something like:
public <T> T unwrap (Class<T> type) {
Object o = this;
while (o != null && !type.isInstance(o)) {
if (o instanceof Wrapper<?>) {
o = ((Wrapper<?>) o).getDelegate();
} else {
o = null;
}
}
return o == null ? null : type.isInstance(o) ? type.cast(o) : null;
}
-Tim
--
You received this message because you are subscribed to a topic in the Google Groups "Netty discussions" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/netty/2v_QzVyMYlE/unsubscribe.
To unsubscribe from this group and all its topics, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/1433906078.358733.291440753.428AF6A6%40webmail.messagingengine.com.
Tim Boudreau
Jun 10, 2015, 4:18:29 AM6/10/15
to ne...@googlegroups.com
I've pushed a rough implementation of everything we've been talking about here to g...@github.com:timboudreau/netty-http-client.git - browse it here:
It seems to work but has had only a tiny bit of testing. But there is:
- A ChannelPool implementation that wraps another as described
- A MultiHostChannelPool which takes a host/port argument
- Wrapper channels/futures/promises that return the wrapper channel where appropriate
- A no-op NullChannelPool
- A wrapper ChannelPool that just keeps timestamps of when it was accessed (so MultiHostChannelPool can clean up not-recently used pools)
- Copied implementations of SimpleChannelPool and FixedChannelPool as discussed
The wrapper code is a bit verbose, but very straightforward.
-Tim
Norman Maurer
Jun 10, 2015, 4:24:01 AM6/10/15
to ne...@googlegroups.com
Comments inline.
2015-06-10 4:27 GMT+02:00 이희승 (Trustin Lee) <t...@motd.kr>:
Hi Tim,Thank you very much for your feed back first of all.On Wed, Jun 10, 2015, at 10:57 AM, Tim Boudreau wrote:I'd been waiting for ChannelPool to appear in Netty, to update my Netty HTTP Client[1] with a connection pool (per-host/port collection of open channels, so keep-alive connections can be reused).So I finally spent a few hours working on that, but the design of ChannelPool does not make it easy. So I'm wondering if this use case just isn't the kind of thing it was meant for (it looks like it was intended for things like JDBC-style connection pools - a bunch of connections to one host, period).Specific issues I'm running into:- SimpleChannelPool calls the no-argument bootstrap.connect(). Easy enough to have a subclass that knows the host+port and have one instance per host+port. However, FixedChannelPool is a final subclass of SimpleChannelPool, so the only way to deal with that is to copy the source code of FixedChannelPool. Better would be to unfinal FixedChannelPool and final most of its methods; better still, design-wise, would be to factor the code that establishes the connection into a separate class.Sounds like a good idea. We could probably introduce an interface that is used to establish a new connection and its default implementation that uses Bootstrap.
So you would inject this into the constructor ?
- SimpleChannelPool calls bootstrap.handler(), clobbering whatever handler was previously set up on it (i.e. one that does something useful!) - replacing it with one that simply calls ChannelPoolHandler on various events (I'm not sure what ChannelPoolHandler is good for - logging?). This is part of a larger issue that SimpleChannelPool assumes it has the Bootstrap to itself, and that is not necessarily true.Introducing a new type dedicated for the establishment of a new connection (your suggestion in the first bullet point) will probably solve this issue together.- ChannelPool.release() is clunky - a cleaner pattern would be for a channel pool to wrap the real Channel object in one that delegates to it, and simply uses calls to close() to release the channel back to the pool. That way you don't have to go find every call that closes a channel in your codebase, give it a reference to the ChannelPool and replace calls to close with pool.release(channel) - which means that a whole lot of code has to be ChannelPool aware that probably shouldn't.Norman and I discussed about this while designing ChannelPool and we decided not to override the close() operation. It's mainly because we don't really know what handles the pipeline contains and it is likely to introduce some conflicts.
Exactly.
- Why return Future<Channel> from ChannelPool.acquire(), instead of ChannelFuture (which is a Future<Void> and therefore cannot be returned by it)? If you're using any of the connect() methods on Bootstrap, you're *going* to get a ChannelFuture, and this just forces everybody to write a bunch of boilerplate to create a promise, listen on the ChannelFuture and update the promise on completion.Returning a ChannelFuture instead of Future<Channel> sounds like a good idea to me.
I thought about this and decided against it. The reason for this is that ChannelFuture.channel() can not be called until the right Channel is acquired from the pool (which will be done async). For Bootstrap.connect() this is not an issue as we create a new Channel even if the connect fails, which is different from what we need t do here (which is return the Channel instance that was prior created).
At this point, I'm considering just writing connection pooling from scratch without this stuff, but if I'm using it wrong and it is intended for this sort of use case, maybe there are some changes that could be made to make the API a little more friendly to it.Thanks again for your feed back. I think you have some valid points and we should address them.
+1
--
You received this message because you are subscribed to the Google Groups "Netty discussions" group.
To unsubscribe from this group and stop receiving emails from it, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/1433903226.342493.291411713.452BAAC4%40webmail.messagingengine.com.
Norman Maurer
Jun 10, 2015, 4:27:08 AM6/10/15
to ne...@googlegroups.com
I will review once I have a few cycles today or tomorrow. Sorry but I'm very busy today :/
--
You received this message because you are subscribed to the Google Groups "Netty discussions" group.
To unsubscribe from this group and stop receiving emails from it, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/625f1483-d1bc-4e27-a3cb-6366853ed620%40googlegroups.com.
Tim Boudreau
Jun 10, 2015, 12:30:04 PM6/10/15
to ne...@googlegroups.com
On Wednesday, June 10, 2015 at 4:24:01 AM UTC-4, Norman Maurer wrote:
- SimpleChannelPool calls the no-argument bootstrap.connect(). Easy enough to have a subclass that knows the host+port and have one instance per host+port. However, FixedChannelPool is a final subclass of SimpleChannelPool, so the only way to deal with that is to copy the source code of FixedChannelPool. Better would be to unfinal FixedChannelPool and final most of its methods; better still, design-wise, would be to factor the code that establishes the connection into a separate class.Sounds like a good idea. We could probably introduce an interface that is used to establish a new connection and its default implementation that uses Bootstrap.So you would inject this into the constructor ?
Yeah, something like this:
used like this
Norman and I discussed about this while designing ChannelPool and we decided not to override the close() operation. It's mainly because we don't really know what handles the pipeline contains and it is likely to introduce some conflicts.Exactly.
This would probably give the best of both worlds - if you want that behavior, just wrap any ChannelPool in a ReleaseOnCloseChannelPool:
- Why return Future<Channel> from ChannelPool.acquire(), instead of ChannelFuture (which is a Future<Void> and therefore cannot be returned by it)? If you're using any of the connect() methods on Bootstrap, you're *going* to get a ChannelFuture, and this just forces everybody to write a bunch of boilerplate to create a promise, listen on the ChannelFuture and update the promise on completion.Returning a ChannelFuture instead of Future<Channel> sounds like a good idea to me.I thought about this and decided against it. The reason for this is that ChannelFuture.channel() can not be called until the right Channel is acquired from the pool (which will be done async). For Bootstrap.connect() this is not an issue as we create a new Channel even if the connect fails, which is different from what we need t do here (which is return the Channel instance that was prior created).
Ok.
-Tim
Norman Maurer
Jun 25, 2015, 4:42:38 AM6/25/15
to ne...@googlegroups.com
Hi Tim,
sorry for the delay :(
I did look at your code but I think wrapping the Channel opens a can of worms. Basically we would need to do a lot of hacks to ensure the "original" Channel never escaped. This includes wrapping:
* ChannelPipeline
* Channel
* ChannelHandlerContext
* ChannelHandler
* ChannelFuture
* ChannelPromise
....
So I think this is not feasible. What I agree with is that I guess we should factor out the connector stuff. Let me come up with a fix for this.
And thanks again for the input!
--
You received this message because you are subscribed to the Google Groups "Netty discussions" group.
To unsubscribe from this group and stop receiving emails from it, send an email to netty+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/netty/8534d60c-c15b-412d-b070-aae4017a09c0%40googlegroups.com.
Tim Boudreau
Jun 25, 2015, 2:53:40 PM6/25/15
to ne...@googlegroups.com
I did look at your code but I think wrapping the Channel opens a can of worms. Basically we would need to do a lot of hacks to ensure the "original" Channel never escaped. This includes wrapping:* ChannelPipeline* Channel* ChannelHandlerContext* ChannelHandler* ChannelFuture* ChannelPromise....So I think this is not feasible.
I agree it is not pretty (and it was not fun to write) - and you are right that it is critical to make sure the original Channel object never escapes.
But at the same time, such wrappers are deterministic, simple to test and prove they work as advertised, and almost simple enough to generate with a script.
If you come up with a prettier solution, I'd welcome it, but the wrapper approach would work perfectly well - it's just not fun stuff to write.
-Tim
Reply all
Reply to author
Forward
0 new messages