Failures in CI across platforms after nogc env update #1573

Activity

1. github-project-automation

   added this to Node-API Team Projecton Sep 4, 2024
2. github-project-automation

   moved this to Need Triage in Node-API Team Projecton Sep 4, 2024
3. mhdawson commented on Sep 4, 2024

   

   mhdawson

   on Sep 4, 2024

   MemberAuthor

   More actions

   @KevinEady is that something you have some cycles to look at ?
4. mhdawson commented on Sep 4, 2024

   

   mhdawson

   on Sep 4, 2024

   MemberAuthor

   More actions

   It looks like env is unused with the default defines, should just need to adjust the parameter definition, looking at it.
5. KevinEady

   self-assigned this

   on Sep 4, 2024
6. KevinEady

   moved this from Need Triage to In Progress in Node-API Team Projecton Sep 4, 2024
7. KevinEady commented on Sep 4, 2024

   

   KevinEady

   on Sep 4, 2024

   Contributor

   More actions

   It looks like env is unused with the default defines, should just need to adjust the parameter definition, looking at it.

   It's not as simple because of the constexpr checks: we only need to use env if the base class defines certain methods.

   The test fails with:

   In file included from ../../napi.h:3257,
                    from ../addon_data.cc:3:
   ../../napi-inl.h: In instantiation of ‘static void Napi::ObjectWrap<T>::FinalizeCallback(node_api_nogc_env, void*, void*) [with T = Addon::VerboseIndicator; node_api_nogc_env = napi_env__*]’:
   

   ... and this is because Addon::VerboseIndicator does not have a Finalize. Since there's no VerboseIndicator::Finalize, the ObjectWrap::FinalizeCallback doesn't use the env parameter.

   Looking at the previous implementation: the ObjectWrap base class defines an empty Finalize() to call with env:
   

   node-addon-api/napi-inl.h

   Lines 4924 to 4932 in bf49519

   	template <typename T>
   	inline void ObjectWrap<T>::FinalizeCallback(napi_env env,
   	void* data,
   	void* /*hint*/) {
   	HandleScope scope(env);
   	T* instance = static_cast<T*>(data);
   	instance->Finalize(Napi::Env(env));
   	delete instance;
   	}

   
   

   node-addon-api/napi-inl.h

   Lines 4837 to 4838 in bf49519

   	template <typename T>
   	inline void ObjectWrap<T>::Finalize(Napi::Env /*env*/) {}

   Should I do something similar...? Seems odd to need to do this just to get rid of the warning... but I could imagine some projects using the same warnings-as-error configuration for this unused-but-set-parameter.
8. KevinEady commented on Sep 4, 2024

   

   KevinEady

   on Sep 4, 2024

   Contributor

   More actions

   Ah, okay ... I had to use the constexpr approach because the delete instance; either happens immediately (if constexpr for "has extended finalizer" condition fails) or gets delayed to the ObjectWrap::PostFinalizeCallback if there is a delayed finalizer.

   Doing something similar to the old approach (ie. defining a no-op, non-basic Finalize) would not work, since the delete instance would then be delayed.

   Soooo not sure exactly how to address.
9. NickNaso commented on Sep 4, 2024

   

   NickNaso

   on Sep 4, 2024

   Member

   More actions

   Could be the following code a possible solution:

   ...
   template <typename T>
   #ifndef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
   inline void ObjectWrap<T>::FinalizeCallback(node_api_nogc_env env,
                                               void* data,
                                               void* /*hint*/) {
   #else
   inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/,
                                               void* data,
                                               void* /*hint*/) {
   
   #endif
   ...
10. mhdawson commented on Sep 4, 2024

    

    mhdawson

    on Sep 4, 2024

    Last edited by mhdawson

    MemberAuthor

    More actions

    @NickNaso I was thinking about something similar to what you suggested except:

    inline void ObjectWrap<T>::FinalizeCallback(
    #ifndef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
        node_api_nogc_env env,
    #else
        node_api_nogc_env /*env*/,
    #endif
        void* data,
        void* /*hint*/) {

    EDIT: but looking at the two I think I prefer the one that @NickNaso suggested.
11. mhdawson commented on Sep 4, 2024

    

    mhdawson

    on Sep 4, 2024

    MemberAuthor

    More actions

    But looking at it I don't think that will work because of the constexpr as the use of env is not just based on the define.
12. mhdawson

    closed this as completedin #1574on Sep 5, 2024
13. github-project-automation

    moved this from In Progress to Done in Node-API Team Projecton Sep 5, 2024