netty: Fix client keepalive initialization (again) #2871
Conversation
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
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public Runnable start(Listener transportListener) { | ||
| lifecycleManager = new ClientTransportLifecycleManager( |
There was a problem hiding this comment.
did you forget to replace the lifecycleManager field with a local variable? Ditto for the keepaliveManager field if it is not exposed to test.
There was a problem hiding this comment.
lifecycleManager is used in statusFromFailedFuture, so it must remain a field. I left keepaliveManager because it is used by the test... although I agree that is questionable given the test didn't catch this bug.
There was a problem hiding this comment.
oops, I see lifecycleManager is also used in statusFromFailedFuture. But why did you add lifecycleManager as an extra argument of newHandler?
There was a problem hiding this comment.
I thought my comment explained that:
Only use fields initialized before start()
Since lifecycleManager is initialized in start(), the flow in start() is more obvious when being passed as an argument. Otherwise you could move newHandler() within start() and not notice that a field is no longer initialized.
There was a problem hiding this comment.
I see. I think move it inside start() is cleaner since it's only one line of code and used only once.
There was a problem hiding this comment.
I see. I think move it inside start() is cleaner since it's only one line of code and used only once.
dapengzhang0
left a comment
dapengzhang0
left a comment
There was a problem hiding this comment.
The fix looks very nice!
|
I've run manual tests and I see keepalive pings! |
2 participants
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
I haven't tested this manually yet. I wanted to send it out to get opinions on whether this is the fix we want. It seems considerably less invasive than some other options we were discussing.