Skip to content

async-work: fix complete callback segv/ticks#380

Merged
yorkie merged 4 commits intomasterfrom
fix/n-api
Oct 21, 2018
Merged

async-work: fix complete callback segv/ticks#380
yorkie merged 4 commits intomasterfrom
fix/n-api

Conversation

@legendecas
Copy link
Copy Markdown
Contributor

@legendecas legendecas commented Oct 20, 2018

Checklist
  • npm test passes
  • tests and/or benchmarks are included

@legendecas legendecas requested review from qile222 and yorkie October 20, 2018 12:15
@legendecas legendecas changed the title fix(async-work): trigger next tick after complete of async work async-work: fix complete callback segv/ticks Oct 20, 2018
If users invoke napi_delete_async_work in their async work complete callback
Copy link
Copy Markdown
Member

@yorkie yorkie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks weird, the implemented copy should fail on this one: https://github.com/Rokid/ShadowNode/blob/1a7a7649be26330cebde42131808142eaede5b3b/test/addons-napi/test_async/test_async.cc#L68, I think we should revisit it.

}
}

iotjs_process_next_tick();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

@legendecas legendecas Oct 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are already included https://github.com/Rokid/ShadowNode/blob/1a7a7649be26330cebde42131808142eaede5b3b/test/napi/napi_async.test.js#L12
Yet it doesn't expose the problem as expected though.

@yorkie
Copy link
Copy Markdown
Member

yorkie commented Oct 20, 2018

Ah, we did skip the test_async/test.js at https://github.com/Rokid/ShadowNode/blob/1a7a7649be26330cebde42131808142eaede5b3b/test/napi-testsets.json#L15

@legendecas Could you please remove the skip flag now?

@legendecas
Copy link
Copy Markdown
Contributor Author

@yorkie it's not possible to enable test_async/test.js pulled from node repo for not implemented sync api
https://github.com/Rokid/ShadowNode/blob/master/test/addons-napi/test_async/test.js#L17

@yorkie
Copy link
Copy Markdown
Member

yorkie commented Oct 20, 2018

it's not possible to enable test_async/test.js pulled from node repo for not implemented sync api

Let's support it, child process Sync APIs should be more useful on our target devices as a script.

@yorkie
Copy link
Copy Markdown
Member

yorkie commented Oct 20, 2018

Or could we have the patches to N-API Test Suite just like the openwrt/buildroot package, which provides a way to cover more NTS without any forks with source?

@mhdawson @gabrielschulhof, I think this tooling could also be included into our future NTS repository, and it helps implementor to get more coverages.

@yorkie
Copy link
Copy Markdown
Member

yorkie commented Oct 21, 2018

@legendecas we should avoid any case that uses freed blocks, so please set the pointer to be null after it gets freed at: https://github.com/Rokid/ShadowNode/blob/c56f4f96bd53259befece9f2ea996fe784f98a7e/src/napi/node_api_async.c#L101

And use IOTJS_ALLOC and IOTJS_RELEASE, too.

Copy link
Copy Markdown
Contributor

@qile222 qile222 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@yorkie yorkie merged commit ddee66f into master Oct 21, 2018
@yorkie yorkie deleted the fix/n-api branch October 21, 2018 08:08
qile222 pushed a commit that referenced this pull request Oct 27, 2018
* master:
  build: use CMAKE_INSTALL_PREFIX instead of custom install command (#391)
  child_process: introduce binary protocol to optimizing performance (#393)
  iotjs: cache parsed dump table for error.stack at runtime (#392)
  jerry: merge a59cf4f from upstream to optimize the function call (#381)
  promise: optimize the performance on .then and static resolve (#385)
  jerry: set prototype/constructor for external functions (#386)
  process: add shebang parser supports (#379)
  napi: fix complete callback segv/ticks on async-work (#380)
  jerry: pass regression-test-issue-2105 for heapdump (#382)
  tls, mqtts: fix connection not be actively disconnected (#375)
  websocket: fix the crash due to http exception (#376)
  process: add signal events support (#372)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants