X Tutup
The Wayback Machine - https://web.archive.org/web/20201231110158/https://github.com/nodejs/node-addon-examples/pull/125
Skip to content
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 an example of combining async work and promises #125

Merged
merged 2 commits into from Feb 27, 2020

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Feb 12, 2020

Copy link
Member

@NickNaso NickNaso left a comment

Hi @legendecas,
could you rename the node-api folder to napi like we tried to do on other examples in the repo?

@legendecas legendecas force-pushed the legendecas:async-work-promise branch from 1a8e0aa to 9bfd515 Feb 12, 2020
@legendecas
Copy link
Member Author

@legendecas legendecas commented Feb 12, 2020

@NickNaso Updated :D

Copy link
Member

@NickNaso NickNaso left a comment

LGTM

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

The N-API calls must be rendered as two lines as described in the comment, otherwise they won't be compiled at all in release mode.

async_work_promise/napi/binding.c Outdated Show resolved Hide resolved
@legendecas legendecas force-pushed the legendecas:async-work-promise branch from 4fee5db to b91b6e6 Feb 13, 2020
@gengjiawen gengjiawen requested a review from gabrielschulhof Feb 21, 2020
@@ -0,0 +1,183 @@
#include <stdio.h>
#include <stdlib.h>
#define NAPI_EXPERIMENTAL

This comment has been minimized.

@gengjiawen

gengjiawen Feb 21, 2020
Member

Nit: is this line redundant ?

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 21, 2020
Contributor

I guess it's unnecessary if every N-API used is already stable.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Why do the primes have to be stored in a linked list? Why not an array of size PRIME_COUNT?

addon_data->box_head is being accessed from two threads. This would require synchronization. However, that would complicate the example. It would be easier to create a structure (perhaps called PromiseData) which would be associated with each work item. It would be allocated in StartWork and deallocated in WorkCompleted. Then, it would be the napi_async_work implementation itself that would provide the synchronization.

Actually, come to think of it, PromiseData could just be the array of primes, initially filled with -1.

@gabrielschulhof gabrielschulhof dismissed their stale review Feb 21, 2020

Changes were made.

@legendecas legendecas force-pushed the legendecas:async-work-promise branch from b91b6e6 to 9b28d48 Feb 23, 2020
NAPI_AUTO_LENGTH,
&work_name) == napi_ok);

// Create a deferred promise which we will resolve at the complete of the work.

This comment has been minimized.

@gabrielschulhof

gabrielschulhof Feb 24, 2020
Contributor

Nit:

Suggested change
// Create a deferred promise which we will resolve at the complete of the work.
// Create a deferred promise which we will resolve at the completion of the work.
Co-Authored-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@mhdawson mhdawson merged commit 1bf2c0a into nodejs:master Feb 27, 2020
@legendecas legendecas deleted the legendecas:async-work-promise branch Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.
X Tutup