Add async_hooks integration for gRPC Server #169
Conversation
kjin
changed the title
| wrap: function(fn) { | ||
| return function() { | ||
| if (resource) { | ||
| resource.emitBefore(); |
There was a problem hiding this comment.
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.
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.
Actually, changing this to let/const -- I think language-level parity with the rest of the code shouldn't be too important right now.
| * @param {grpc.Metadata} metadata Metadata from the client | ||
| */ | ||
| function handleUnary(call, handler, metadata) { | ||
| var asyncResource = createAsyncResourceWrapper('GrpcServerRequest'); |
There was a problem hiding this comment.
As suggested by the official docs, it would be best to use the module name as a prefix. Something like 'grpc.ServerRequest'.
| if (trailer) { | ||
| err.metadata = trailer; | ||
| asyncResource.wrap(function() { | ||
| handler.func(stream, function(err, value, trailer, flags) { |
There was a problem hiding this comment.
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.
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.on('finish', asyncResource.destroy); | ||
| stream.waitForCancel(); | ||
| handler.func(stream); | ||
| asyncResource.wrap(function() { |
| } else { | ||
| sendUnaryResponse(call, value, handler.serialize, trailer, flags); | ||
| } | ||
| asyncResource.destroy(); |
There was a problem hiding this comment.
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.
It's not okay 🙃 I've added additional calls to destroy if the user function is never called.
kjin
force-pushed
the
native-async-hooks
branch
2 times, most recently
from
bcb1ca5 to
b9ba5a4
Compare
kjin
force-pushed
the
native-async-hooks
branch
from
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. |
4 participants
This change adds
async_hooksintegration when it's available (Node 8+), and aims to make almost no impact otherwise. A short explanation of whichAsyncResourceobjects are created is at the top ofserver.js.As the
async_hooksdocs are presently a little hard to digest, I'll try to summarize it here (please correct me if I am mistaken):async_hooksgives us that information by allowing us to add hooks for:init)before)after)destroy)AsyncResourceAPI (anAsyncResourceis an object associated with an asynchronous task) which:initeventemitBefore,emitAfter, andemitDestroywhich corresponds to the ordered list above