fix: call base basic finalizer if none defined by KevinEady · Pull Request #1574 · nodejs/node-addon-api

KevinEady · 2024-09-04T21:19:32Z

Call no-op ObjectWrap<T>::Finalize if user does not provide a Finalizer method.

mhdawson · 2024-09-04T21:25:55Z

 #endif

 instance->Finalize(Napi::BasicEnv(env));
+ } else {


I see how this fixes the existing, not used error, but does it also fix running some finalization that should have run before but would not have, or now call the method just to fix the not used error?

but does it also fix running some finalization that should have run before but would not have
This is not the case. If the user defined their own MyClass::Finalize(BasicEnv), the constexpr (details::HasBasicFinalizer<T>::value) would be true, and then call instance->Finalize(Napi::BasicEnv(env)).
... now call the method just to fix the not used error
This is the case, so it is adding an inefficiency (perhaps the compiler optimizes it out)

My questions is to make sure the additional invocation is desired, if so good, sounds like we found a potential bug as well. If not I think we can just add a (void) env; early in the function to mark it as used to address the warning.

I'm trying the same solution you proposed adding (void) env; on my local machine.

it worked well on my local machine.

codecov-commenter · 2024-09-04T21:27:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.40%. Comparing base (b4aeecb) to head (72655c6).
Report is 2 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #1574 +/- ##
=======================================
Coverage 64.40% 64.40% =======================================
Files 3 3 Lines 2003 2003 Branches 693 693 =======================================
Hits 1290 1290 Misses 146 146 Partials 567 567

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

KevinEady · 2024-09-04T21:37:35Z

I've started a single Jenkins v22 run to see if the fix works

Version	Job	Status
v22.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9288/	✅

NickNaso · 2024-09-04T22:13:29Z

Version	Job	Status
v22.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9289/	🔴
v22.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9290/	✅

The CI fail, but it seems to be systemec failure:

Started by upstream project "node-test-node-addon-api-new
" build number 9289
originally caused by:
Started by user Nicola Del Gobbo
Running as SYSTEM
[EnvInject] - Loading node environment variables.
Building remotely on test-osuosl-rhel8-ppc64_le-1
(v8test rhel8-ppc64le) in workspace /home/iojs/build/workspace/node-test-node-addon-api-new/nodes/rhel8-ppc64le
FATAL: java.nio.channels.ClosedChannelException
java.nio.channels.ClosedChannelException
Also: hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 140-211-168-183-openstack.osuosl.org/140.211.168.183:52928
at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1826)
at hudson.remoting.Request.call(Request.java:199)
at hudson.remoting.Channel.call(Channel.java:1041)
at hudson.FilePath.act(FilePath.java:1229)
at hudson.FilePath.act(FilePath.java:1218)
at hudson.FilePath.mkdirs(FilePath.java:1409)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1241)
at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:649)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:85)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:521)
at hudson.model.Run.execute(Run.java:1894)
at PluginClassLoader for matrix-project//hudson.matrix.MatrixRun.run(MatrixRun.java:153)
at hudson.model.ResourceController.execute(ResourceController.java:101)
at hudson.model.Executor.run(Executor.java:446)
Caused: hudson.remoting.RequestAbortedException
at hudson.remoting.Request.abort(Request.java:346)
at hudson.remoting.Channel.terminate(Channel.java:1122)
at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:241)
at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:221)
at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:825)
at org.jenkinsci.remoting.protocol.FilterLayer.onRecvClosed(FilterLayer.java:289)
at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.onRecvClosed(SSLEngineFilterLayer.java:168)
at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:825)
at org.jenkinsci.remoting.protocol.NetworkLayer.onRecvClosed(NetworkLayer.java:155)
at org.jenkinsci.remoting.protocol.impl.NIONetworkLayer.ready(NIONetworkLayer.java:180)
at org.jenkinsci.remoting.protocol.IOHub$OnReady.run(IOHub.java:789)
at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
at jenkins.util.ErrorLoggingExecutorService.lambda$wrap$0(ErrorLoggingExecutorService.java:51)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
at java.base/java.lang.Thread.run(Thread.java:840)
Collecting metadata...
Metadata collection done.
Notifying upstream projects of job completion
Finished: FAILURE

I try to launch new job.

NickNaso · 2024-09-05T05:45:10Z

The CI is green now I try to launch it for all LTS and main

CI:

Version	Job	Status
main	https://ci.nodejs.org/job/node-test-node-addon-api-new/9291/	✅
v22.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9282/	✅
v20.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9293/	✅
v18.x	https://ci.nodejs.org/job/node-test-node-addon-api-new/9294/	✅

mhdawson

LGTM

mhdawson · 2024-09-05T17:26:00Z

Looks like CIs are good. @NickNaso thanks for kicking those off and checking the solution. Going to land.

mhdawson · 2024-09-05T17:28:05Z

Will check the regular node-addon ci tomorrow to confirm all locks good. @KevinEady, @NickNaso thanks for the quick work to get this addressed.

fix: call base basic finalizer if none defined
427ef4d

mhdawson reviewed Sep 4, 2024
View reviewed changes

discard env value
72655c6

NickNaso approved these changes Sep 5, 2024
View reviewed changes

KevinEady marked this pull request as ready for review September 5, 2024 17:20

mhdawson approved these changes Sep 5, 2024
View reviewed changes

mhdawson merged commit 294a43f into nodejs:mainSep 5, 2024

github-actionsBot mentioned this pull request Sep 9, 2024
chore: release v8.2.0 #1571
Merged

snyk-ioBot mentioned this pull request Nov 10, 2024
[Snyk] Upgrade node-addon-api from 3.2.1 to 8.2.1 doperiddle/bch-rpc-explorer#9
Closed

Navigation Menu

Uh oh!

Uh oh!

fix: call base basic finalizer if none defined - #1574

fix: call base basic finalizer if none defined#1574
mhdawson merged 2 commits into
nodejs:mainfrom
KevinEady:fix-unused-param

KevinEady commented Sep 4, 2024

Uh oh!

mhdawsonSep 4, 2024

Uh oh!

KevinEadySep 4, 2024

Uh oh!

mhdawsonSep 4, 2024

Uh oh!

NickNasoSep 4, 2024•
edited
Loading

Uh oh!

NickNasoSep 4, 2024

Uh oh!

codecov-commenter commented Sep 4, 2024•
edited
Loading

Uh oh!

KevinEady commented Sep 4, 2024•
edited
Loading

Uh oh!

NickNaso commented Sep 4, 2024•
edited
Loading

Uh oh!

NickNaso commented Sep 5, 2024•
edited by KevinEady
Loading

Uh oh!

mhdawson left a comment

Uh oh!

mhdawson commented Sep 5, 2024

Uh oh!

mhdawson commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

KevinEady commented Sep 4, 2024

Uh oh!

mhdawsonSep 4, 2024

Choose a reason for hiding this comment

Uh oh!

KevinEadySep 4, 2024

Choose a reason for hiding this comment

Uh oh!

mhdawsonSep 4, 2024

Choose a reason for hiding this comment

Uh oh!

NickNasoSep 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickNasoSep 4, 2024

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

KevinEady commented Sep 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickNaso commented Sep 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NickNaso commented Sep 5, 2024• edited by KevinEady Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 5, 2024

Uh oh!

mhdawson commented Sep 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NickNasoSep 4, 2024•
edited
Loading

codecov-commenter commented Sep 4, 2024•
edited
Loading

KevinEady commented Sep 4, 2024•
edited
Loading

NickNaso commented Sep 4, 2024•
edited
Loading

NickNaso commented Sep 5, 2024•
edited by KevinEady
Loading