Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KeepAliveManager is not apply to ConnectionHandler in netty transport #2828

Closed
bobwenx opened this issue Mar 16, 2017 · 4 comments · Fixed by #2871
Closed

KeepAliveManager is not apply to ConnectionHandler in netty transport #2828

bobwenx opened this issue Mar 16, 2017 · 4 comments · Fixed by #2871
Assignees
@ejona86
Milestone
1.3

Comments

@bobwenx
Copy link

bobwenx commented Mar 16, 2017 •
edited

What version of gRPC are you using?

GRPC 1.2.0

What JVM are you using (java -version)?

1.8.0_112

What did you do?

I am try to make KeepAlive work with netty transport just now(grpc-java 1.2.0), and after some function test, i think the KeepAlive feature still not work functionally.

Debugging the code(io.grpc.netty.NettyClientTransport#start):
image

So, we known the KeepAliveManager only make effective in io.grpc.netty.NettyClientHandler instance. But the code showing the creation of io.grpc.netty.NettyClientHandler always receive a null keepAliveManager.

image

Probably because the keepAliveManager field is assigning in the end of io.grpc.netty.NettyClientTransport#start() call.

Associated commit: #2729: the commit resolve the problem of NPE(#2726)

cc @lukaszx0 @ejona86
@dapengzhang0
Copy link
Member

dapengzhang0 commented Mar 16, 2017 •
edited

thanks for the notice.
Seems happened after #2729, will fix it.

@dapengzhang0 dapengzhang0 self-assigned this Mar 16, 2017
dapengzhang0 added a commit to dapengzhang0/grpc-java that referenced this issue Mar 16, 2017
@dapengzhang0
netty: fix KeepAliveManager not apply to ConnectionHandler
0534f79 
resolves grpc#2828

- moved construction of keepAliveManager right after channel is constructed.
- pass keepAliveManager into NettyClientHandler by a setter
@dapengzhang0 dapengzhang0 mentioned this issue Mar 16, 2017
Closed
@ejona86 ejona86 assigned ejona86 and unassigned dapengzhang0 Mar 31, 2017
ejona86 added a commit to ejona86/grpc-java that referenced this issue Mar 31, 2017
@ejona86
netty: Fix client keepalive initialization (again)
fe4fb4d 
d116cc9 fixed the NPE, but the initialization of the manager happened
_after_ newHandler() was called, so a null manager was passed to the
handler.

Fixes grpc#2828
@ejona86 ejona86 mentioned this issue Mar 31, 2017
Merged
ejona86 added a commit that referenced this issue Apr 1, 2017
@ejona86
netty: Fix client keepalive initialization (again)
f9eb545 
d116cc9 fixed the NPE, but the initialization of the manager happened
_after_ newHandler() was called, so a null manager was passed to the
handler.

Fixes #2828
@ejona86 ejona86 added this to the 1.3 milestone Apr 5, 2017
@bobwenx
Copy link
Author

bobwenx commented Apr 27, 2017

Hi there! Any plan to release 1.3.x? KeepAlive support is a important feature for us :)

@ejona86
Copy link
Member

ejona86 commented Apr 27, 2017

@bobwenx, 1.3.0 is now available. It's not yet showing up on search.maven.org, but it is available for download. We've not yet made the release announcement, but it's in the works.

@bobwenx
Copy link
Author

bobwenx commented May 6, 2017

@ejona86 maybe a related issue: #2982

@lock lock bot locked as resolved and limited conversation to collaborators Sep 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@ejona86 ejona86

Labels
None yet
Projects
None yet
Milestone
1.3
3 participants
@bobwenx @ejona86 @dapengzhang0