feat: DH-18086: gRPC transport implementation for nodejs backed by http2 #2339
Conversation
bmingles
force-pushed
the
DH-18086-2_http2-grpc-transport
branch
from
eb3a171 to
c25f9cc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2339 +/- ##
=======================================
Coverage 46.70% 46.70%
=======================================
Files 704 704
Lines 39044 39044
Branches 9757 9757
=======================================
Hits 18234 18234
Misses 20799 20799
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bmingles
requested review from
a team and
vbabich
and removed request for
a team
| cancel(): void { | ||
| logger.debug('cancel'); | ||
| assertNotNull(this.request, 'request is required'); | ||
| this.request.close(); |
There was a problem hiding this comment.
Should we reset request to null here? Probably not needed, if request.write handles closed requests fine.
There was a problem hiding this comment.
Good question. @niloc132 do you have any thoughts on this one?
There was a problem hiding this comment.
It looks safe to me to not null out - but nulling it out does come with the benefit of clearly showing why an error happened if it is reused somehow. I'm certainly not against that style of defensive coding, but I think we'd want to update a few other places too to reflect this.
GC-wise, I don't see any issue with the current implementation.
There was a problem hiding this comment.
Opting to leave as-is for now to avoid adding complexity. Setting to null will require changes to start which assumes this stays set to know if it has already been called.
| cancel(): void { | ||
| logger.debug('cancel'); | ||
| assertNotNull(this.request, 'request is required'); | ||
| this.request.close(); |
There was a problem hiding this comment.
It looks safe to me to not null out - but nulling it out does come with the benefit of clearly showing why an error happened if it is reused somehow. I'm certainly not against that style of defensive coding, but I think we'd want to update a few other places too to reflect this.
GC-wise, I don't see any issue with the current implementation.
| @@ -177,7 +177,6 @@ export class NodeHttp2gRPCTransport implements GrpcTransport { | |||
| logger.debug('cancel'); | |||
| assertNotNull(this.request, 'request is required'); | |||
| this.request.close(); | |||
| this.request = null; | |||
There was a problem hiding this comment.
@vbabich I ended up reverting this as Colin pointed out this breaks the check in the start method to avoid calling start multiple times. We could make the state logic smarter, but I think it will introduce complexity that can't easily be tested. Probably best to leave simple for now, unless we run into any particular problems.
DH-18086, DH-18085: gRPC transport implementation for nodejs backed by http2
Note that this is not consumed anywhere in the web but will be used by the vscode extension and tested on
deephaven/vscode-deephaven#202