fix: cannot send ping without ref

[alexander-joinlane]

The logger statements rely on this.channelzRef.id which is possibly undefined depending on when execution occurs.

In order to avoid possibly undefined .id, ensure you can only send ping if channelzRef exists

Hey all, encountered a problem like this in usage. Just a suggested change, I'm not planning on following-up on this I'm afraid. We're planning to downgrade dependencies locally.

[linux-foundation-easycla]

• ❌ - login: @alexander-joinlane / name: Alexander Teno . The commit (f1d6b90) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

[murgatroid99]

How did you get into a situation where this is an issue? The channelzRef is unconditionally set in the constructor, and there's no code that unsets it.

[murgatroid99]

If you experienced an error please file an issue describing it.

[devshah1]

@murgatroid99 This commit led to failures in our application with the following error as one of our dependencies pulls in the latest for this package -

Version: 1.8.19

Package dep graph
@google-cloud/pubsub@^3.0.1" > google-gax@^3.5.2 > "@grpc/grpc-js" "~1.8.0"

Error log -

/node_modules/@grpc/grpc-js/src/transport.ts in Http2Transport.keepaliveTrace at line 255:75
System
/node_modules/@grpc/grpc-js/src/transport.ts in Http2Transport.maybeStartKeepalivePingTimer at line 367:12
System
/node_modules/@grpc/grpc-js/src/transport.ts in new Http2Transport at line 151:12
System
/node_modules/@grpc/grpc-js/src/transport.ts in ClientHttp2Session.<anonymous> at line 622:17
System

node:events in Object.onceWrapper at line 628:26
System
node:events in ClientHttp2Session.emit at line 513:28
System
node:domain in ClientHttp2Session.emit at line 489:12
System
node:internal/http2/core in emit at line 329:3
System
node:internal/process/task_queues in processTicksAndRejections at line 85:21

[murgatroid99]

I see what the problem is. I'll get a patch out today to fix it. In the meantime, if you are setting the option grpc.keepalive_permit_without_calls, you can avoid this error by not setting that option.

[devshah1]

@murgatroid99 Thanks a bunch for the quick response and suggested workaround!

[murgatroid99]

In fact, for anyone who had that option enabled, due to other improvements to keepalive behavior in grpc-js 1.8.19, you may no longer need it. I recommend re-evaluating whether the keepalive behavior meets your needs without that option set.

[alexander-joinlane]

How did you get into a situation where this is an issue? The channelzRef is unconditionally set in the constructor, and there's no code that unsets it.

@murgatroid99 I can't say for sure how exactly we encounter it, it's behind multiple libraries (nice-grpc) and also occurring behind the processTicksAndRejections, it began after a recent deployment when we think there was a peer dependency update that we don't manage.

Thankfully it sounds like you have an idea, my theory was the ping behaviour was queued and executed before the channelzRef was set in the constructor().

If this work doesn't make sense, please feel free to hijack the PR in whatever way you like 😄

[murgatroid99]

Thankfully it sounds like you have an idea, my theory was the ping behaviour was queued and executed before the channelzRef was set in the constructor().

You are correct. The fix for that is to rearrange those actions in the constructor: #2519.

[alexander-joinlane]

Closing in favor of #2519

[murgatroid99]

The fix for this is now out in version 1.8.20