fix: call base basic finalizer if none defined #1574

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

napi-inl.h

@@ -5023,6 +5023,11 @@ inline void ObjectWrap<T>::FinalizeCallback(node_api_nogc_env env,
 #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:main Sep 5, 2024
48 checks passed

github-actions bot mentioned this pull request Sep 9, 2024

chore: release v8.2.0 #1571

Merged

snyk-io bot 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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: call base basic finalizer if none defined #1574

fix: call base basic finalizer if none defined #1574

KevinEady commented Sep 4, 2024

mhdawson Sep 4, 2024

KevinEady Sep 4, 2024

mhdawson Sep 4, 2024

NickNaso Sep 4, 2024 •

edited

Loading

NickNaso Sep 4, 2024

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

mhdawson left a comment

mhdawson commented Sep 5, 2024

mhdawson commented Sep 5, 2024

fix: call base basic finalizer if none defined #1574

fix: call base basic finalizer if none defined #1574

Conversation

KevinEady commented Sep 4, 2024

mhdawson Sep 4, 2024

Choose a reason for hiding this comment

KevinEady Sep 4, 2024

Choose a reason for hiding this comment

mhdawson Sep 4, 2024

Choose a reason for hiding this comment

NickNaso Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

NickNaso Sep 4, 2024

Choose a reason for hiding this comment

codecov-commenter commented Sep 4, 2024 • edited Loading

Codecov Report

KevinEady commented Sep 4, 2024 • edited Loading

NickNaso commented Sep 4, 2024 • edited Loading

NickNaso commented Sep 5, 2024 • edited by KevinEady Loading

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 5, 2024

mhdawson commented Sep 5, 2024

NickNaso Sep 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