Removing transport shutdown hooks from channel builder #837
Conversation
|
The main issue with this PR ATM is that because the channel only creates a single transport at a time, the shared executor will be repeatedly created and destroyed if using a single Channel. Need to put some thought into how to better handle that ... or if it is really going to matter much, especially once we add load balancing. |
| } finally { | ||
| // If we are using the shared event loop group, we need to return our reference. | ||
| if (usingSharedGroup) { | ||
| SharedResourceHolder.release(Utils.DEFAULT_WORKER_EVENT_LOOP_GROUP, group); |
There was a problem hiding this comment.
This must only be called once. It should observe notifyTerminated.
There was a problem hiding this comment.
Ack ... this has changed.
|
@madongfly @ejona86 @zhangkun83 @carl-mastrangelo PTAL ... I've reworked this to add reference counting of |
|
@nmittler LGTM |
| @@ -98,7 +100,7 @@ | |||
| */ | |||
| public ServerImpl(Executor executor, HandlerRegistry registry, | |||
|
@nmittler LGTM |
|
LGTM |
|
@ejona86 gentle ping... |
| @@ -380,10 +401,8 @@ public void transportTerminated() { | |||
| log.warning("transportTerminated called after already terminated"); | |||
| } | |||
| terminated = true; | |||
| onChannelTerminated(); | |||
There was a problem hiding this comment.
nit: keep ordering the same between this and shutdown(). notifyAll() and onChannelTerminated() are swapped.
|
@nmittler LGTM. My comment for AbstractReferenceCounted is the most important one. |
|
@ejona86 PTAL |
|
@nmittler, LGTM. |
nmittler
force-pushed
the
transport_factory
branch
2 times, most recently
from
a4dee9f
to
1d83d43
Compare
The current process of building a channel is a bit complicated in that transports have to provide a own shutdown hook to the channel builder in order to close shared executors. This somewhat entagled creation pattern makes it difficult to separate the process of channel building from transport building. Better separating these two should make the code more readable and maintainable moving forward.
nmittler
force-pushed
the
transport_factory
branch
from
1d83d43
to
cfc0912
Compare
|
Cherry-picked as 777e928 FYI @zhangkun83 this should help with your load balancing work |
The current process of building a channel is a bit complicated in that transports have to provide a own shutdown hook to the channel builder in order to close shared executors. This somewhat entagled creation pattern makes it difficult to separate the process of channel building from transport building. Better separating these two should make the code more readable and maintainable moving forward.