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

Jest and why-is-node-running reports open handle on exit due to InstanceData.drop_queue's threadsafe function #948

Open
mjameswh opened this issue Nov 28, 2022 · 1 comment · May be fixed by #950

Comments

@mjameswh
Copy link

Description

In Napi-6 mode, InstanceData registers a drop_queue threadsafe function, which ensures that leaked resources correctly get disposed. However, that TSFN itself is being reported by Jest, why-is-node-running and others (simply "Jest" thereafter) as leaked resources, since that function is never destroyed.

How to reproduce

InstanceData is lazily initialized. The issue can be observed by running any Jest test that forces initialization of InstanceData, notably by creating a Root object or calling cx.channel(). For example:

In rust:

pub fn test_inner_data_drop_queue_leak(mut cx: FunctionContext) -> JsResult<JsObject> {
    let some_object = cx.empty_object().root(&mut cx).into_inner(&mut cx);
    Ok(some_object)
}

In JavaScript:

test('httpWorkflow with mock activity', () => {
      (native as any).testInnerDataDropQueueLeak();
});

Then running using:

npx jest --detectOpenHandles

Results in this Jest warning message:

Jest has detected the following 1 open handle potentially keeping Jest from exiting:

  ●  neon threadsafe function

Discussion and Proposed solution

This is technically a false-positive, since the TSFN is .unref() so that it will not keep the process alive. Unfortunately, Node's async hook callbacks API doesn't provide an official way to know that an async resource has been unref(). There is therefore no way for Jest to know that the TSFN can safely be ignored, by relying on Node's API alone.

Instead, Jest looks at the async resource for the presence of a hasRef() function. If that function exists on the async resource object, then Jest will call that function to determine if the resource is still active. why-is-node-running also support that function, and Node itself has added this function on some of its own resources specifically for the purpose of helping Jest and the like properly detect leaks.

It is therefore suggested that a hasRef() function should be added to the async resource associated TSFNs. That function would simply return either true or false, depending on either the TSFN is referenced() or unref().

@kjvalencik
Copy link
Member

Thanks for sharing this detailed report @mjameswh!

Neon does not currently provide an async_resource when creating a Threadsafe function. That gives Neon an opportunity to add this functionality. We need to ensure it is fairly lightweight since there is not currently a use case beyond helping to remove this false positive. This shouldn't be too much of a concern since creating threadsafe function must be infrequent for a high performing add-on.

One option:

  • Create a hasRef JsFunction and keep a copy of it in InstanceData
  • Add a is_ref: Arc<Mutex<bool>> to ThreadsafeFunction
  • Mutate the is_ref when ref and unref is called
  • Create an object to use as async_resource in each ThreadsafeFunction
  • Use napi_wrap to attach is_ref to the async resource
  • Add a hasRef key to a function that will use napi_unwrap on this and check if it is referenced

This will allow tools that check for hasRef to be able to tell if Neon threadsafe functions are referenced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants