Skip to content

[v14.x-backport] node-api: rtn pending excep on napi_new_instance#39516

Closed
legendecas wants to merge 1 commit intonodejs:v14.x-stagingfrom
legendecas:backport-to-14/38798
Closed

[v14.x-backport] node-api: rtn pending excep on napi_new_instance#39516
legendecas wants to merge 1 commit intonodejs:v14.x-stagingfrom
legendecas:backport-to-14/38798

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Jul 25, 2021

When there are any JavaScript exceptions pending,
napi_pending_exception should be returned.

PR-URL: #38798
Reviewed-By: Anna Henningsen anna@addaleax.net
Reviewed-By: Michael Dawson midawson@redhat.com

@legendecas legendecas requested review from addaleax and mhdawson July 25, 2021 16:52
@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v14.x labels Jul 25, 2021
@legendecas legendecas changed the title node-api: rtn pending excep on napi_new_instance [v14.x-backport] node-api: rtn pending excep on napi_new_instance Jul 25, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jul 25, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/39260/

addaleax
addaleax approved these changes Jul 25, 2021
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

So … this looks good, but:

When I reviewed the original PR, the reason I approved that this was a bugfix that aligned this call site with other call sites.

However, this prompted me to take another look at the source, it seems that CHECK_MAYBE_EMPTY most often uses napi_generic_failure – which seems pretty wrong? An empty maybe by definition indicates a pending exception.

@legendecas
Copy link
Member Author

legendecas commented Jul 26, 2021 •
edited
Loading

@addaleax previously we consider this returning status change as a possible ABI breaking change. However, the original PR does resolve a real problem, which we considered as a bug of node core. I believe it is time to re-evaluate if such a change is breaking anymore. If it is not been categorized as an ABI breaking change, we should apply the change on all other similar call sites.

/cc @nodejs/node-api

@legendecas legendecas added the node-api Issues and PRs related to the Node-API. label Jul 26, 2021
@addaleax
Copy link
Member

addaleax commented Jul 26, 2021

@legendecas Yeah, I think this is either an all-or-nothing situation.

However, the original PR does resolve a real problem

But the same goes for the other call sites…?

mhdawson
mhdawson approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@targos
Copy link
Member

targos commented Aug 8, 2021

Can you please rebase?

@targos
Copy link
Member

targos commented Aug 29, 2021

ping @legendecas

@legendecas legendecas force-pushed the backport-to-14/38798 branch from f89cf6a to b55b2f9 Compare August 30, 2021 16:14
@legendecas
Copy link
Member Author

legendecas commented Aug 30, 2021

@targos updated! :)

@legendecas @targos
node-api: rtn pending excep on napi_new_instance
e999999 
When there are any JavaScript exceptions pending,
`napi_pending_exception` should be returned.

PR-URL: nodejs#38798
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos force-pushed the backport-to-14/38798 branch from b55b2f9 to e999999 Compare September 1, 2021 17:34
targos pushed a commit that referenced this pull request Sep 3, 2021
@legendecas @targos
node-api: rtn pending excep on napi_new_instance
a74032a 
When there are any JavaScript exceptions pending,
`napi_pending_exception` should be returned.

PR-URL: #38798
Backport-PR-URL: #39516
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos
Copy link
Member

targos commented Sep 3, 2021

Landed in a74032a

@targos targos closed this Sep 3, 2021
@legendecas legendecas deleted the backport-to-14/38798 branch March 25, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@legendecas @nodejs-github-bot @addaleax @targos @mhdawson