-
Notifications
You must be signed in to change notification settings - Fork 648
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.
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
Add async_hooks integration for gRPC Server #169
Conversation
wrap: function(fn) { | ||
return function() { | ||
if (resource) { | ||
resource.emitBefore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I am changing this to be safer here: nodejs/node#18513. But this is ought to be fine for now.
if (resource) { | ||
resource.emitBefore(); | ||
try { | ||
var result = fn.apply(this, arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: declare var at the top so that it doesn't seem to escape from the scope (even though that's how var is actually implemented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, changing this to let
/const
-- I think language-level parity with the rest of the code shouldn't be too important right now.
@@ -567,14 +576,15 @@ ServerDuplexStream.prototype.waitForCancel = waitForCancel; | |||
* @param {grpc.Metadata} metadata Metadata from the client | |||
*/ | |||
function handleUnary(call, handler, metadata) { | |||
var asyncResource = createAsyncResourceWrapper('GrpcServerRequest'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As suggested by the official docs, it would be best to use the module name as a prefix. Something like 'grpc.ServerRequest'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (trailer) { | ||
err.metadata = trailer; | ||
asyncResource.wrap(function() { | ||
handler.func(stream, function(err, value, trailer, flags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use isn't making sense to me. handler.func
seems to be getting executed in the same continuation as handleClientStreaming
– it is called synchronously.
It doesn't seem logical to create an AsyncResource
to wrap a synchronously executed function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to mirror what the other handler types do. I don't think there's harm in having nesting here (from the perspective of an agent listening for events with only one type of async resource, there is no concept of nesting) -- and I can't think of a good alternative without changing the behavior of the code to move the invocation handler.func
to the next turn of the event loop. At the minimum we will definitely want a per-request grpc.ServerRequest
-type async resource. The grpc.Server
async resource doesn't seem strictly required at the moment but it follows the precedent set by http
.
stream.waitForCancel(); | ||
handler.func(stream); | ||
asyncResource.wrap(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likewise.
@@ -598,8 +608,9 @@ function handleUnary(call, handler, metadata) { | |||
} else { | |||
sendUnaryResponse(call, value, handler.serialize, trailer, flags); | |||
} | |||
asyncResource.destroy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on why it is okay that the destroy doesn't get called on all paths?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not okay 🙃 I've added additional calls to destroy
if the user function is never called.
bcb1ca5
to
b9ba5a4
Compare
b9ba5a4
to
373de3e
Compare
Closing this for now since #234 was landed and released. I've made a note to self to check what needs to be done in the JS layer, if any. |
This change adds
async_hooks
integration when it's available (Node 8+), and aims to make almost no impact otherwise. A short explanation of whichAsyncResource
objects are created is at the top ofserver.js
.As the
async_hooks
docs are presently a little hard to digest, I'll try to summarize it here (please correct me if I am mistaken):async_hooks
gives us that information by allowing us to add hooks for:init
)before
)after
)destroy
)AsyncResource
API (anAsyncResource
is an object associated with an asynchronous task) which:init
eventemitBefore
,emitAfter
, andemitDestroy
which corresponds to the ordered list above