fix: avoid using deprecated Nan APIs #234
Conversation
|
Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement. After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.
Regards, |
| @@ -252,6 +253,7 @@ NAN_METHOD(SetDefaultLoggerCallback) { | |||
| grpc_logger_state.logger_set = true; | |||
| } | |||
| grpc_logger_state.callback = new Nan::Callback(info[0].As<v8::Function>()); | |||
| grpc_logger_state.async_resource = new Nan::AsyncResource("grpc:logger"); | |||
There was a problem hiding this comment.
I didn't find a delete of grpc_logger_state.callback, but I didn't look too hard either. Wherever we free one, we should free the other.
| @@ -318,8 +318,9 @@ NAN_METHOD(Server::TryShutdown) { | |||
| Server *server = ObjectWrap::Unwrap<Server>(info.This()); | |||
| if (server->wrapped_server == NULL) { | |||
| // Server is already shut down. Call callback immediately. | |||
There was a problem hiding this comment.
Note that this looks like an instance of Zalgo. Its an edge case though.
|
Hello @thelinuxfoundation bot. Please re-verify. |
| @@ -245,11 +245,10 @@ int plugin_get_metadata( | |||
| grpc_metadata creds_md[GRPC_METADATA_CREDENTIALS_PLUGIN_SYNC_MAX], | |||
| size_t *num_creds_md, grpc_status_code *status, | |||
| const char **error_details) { | |||
| HandleScope scope; | |||
There was a problem hiding this comment.
I don't think we need a HandleScope here because this function doesn't call any Node APIs.
There was a problem hiding this comment.
Actually the AsyncResource constructor allocates a resource object if one is not passed in. Arguably this could be fixed in Nan instead, but for now, this is the API we have.
There was a problem hiding this comment.
Fix in Nan: nodejs/nan#757 which will take some time to review / land / release. For now let's use a HandleScope ourselves.
packages/grpc-native-core/ext/call.h
Outdated
| @@ -98,7 +98,7 @@ class Op { | |||
| }; | |||
|
|
|||
| typedef std::vector<unique_ptr<Op>> OpVec; | |||
| struct tag { | |||
| struct tag : public Nan::AsyncResource { | |||
There was a problem hiding this comment.
Why have tag inherit from Nan::AsyncResource, instead of just having a Nan::AsyncResource member?
There was a problem hiding this comment.
Memory management is easier. No need to explicitly delete tag->async_resource. I don't have strong feelings though.
There was a problem hiding this comment.
Other reason: tag represents an async operation, so it makes sense for it to be a subclass of AsyncResource.
There was a problem hiding this comment.
We don't explicitly delete any of the members. They get deleted by default when the tag object is deleted. And subclassing merely for the purpose of categorization isn't really compelling to me, so I would prefer to use composition here.
There was a problem hiding this comment.
Composition it is.
Documented here. These represent the type of asynchronous operation that caused an async hop. This module introduces async hops of its own via direct usage of the uv event loops. The specified type string will be provided to any debug or diagnostic tool that is monitoring async execution via async_hooks. Ultimately this would be provided to the user where they would be able to distinguish async hops due to timers, net, etc. vs. gRPC. |
|
@murgatroid99 I am unable to reproduce the test failures locally. Any ideas? |
|
I made some changes in #248 that touch the same code that this PR touches, and I think that PR also likely fixed the crashes you are seeing here. So if you could merge from master, I think you can get this change to work. |
Starting with Nan 2.9.x certain Nan::Callback::Call APIs are deprecated. Instead there are mechanisms in place that allow native modules to preserve async context across async calls.
WIP, but opening early to get early feedback, and to kick off the CI. I intend to revert the
-Wno-deprecated-declarationsremoval before this lands.Starting with Nan 2.9.x certain Nan::Callback::Call APIs are
deprecated. Instead there are mechanisms in place that allow native
modules to preserve async context across async calls.