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

Neon background task calls fail when run from the REPL if the domain module has been required #418

Closed
apendleton opened this issue Jul 19, 2019 · 7 comments · Fixed by #498

Comments

@apendleton
Copy link

It appears that the following combination of things:

  • the calling of a neon background task
  • the domain module having been first required
  • execution of the whole script from the REPL, or with node -e
  • Node 10.x (or at least, 10.15.3 and 10.16.0)

causes the callback to fail to be called, and instead, for a weird, inscrutable error to be produced:

domain.js:119
    domain.enter();
           ^

TypeError: domain.enter is not a function
    at topLevelDomainCallback (domain.js:119:12)

To reproduce, create a new neon project and edit src/lib.rs to contain:

#[macro_use]
extern crate neon;

use neon::prelude::*;

struct BackgroundTask;

impl Task for BackgroundTask {
    type Output = ();
    type Error = String;
    type JsEvent = JsUndefined;

    fn perform(&self) -> Result<(), String> {
        println!("hello");
        Ok(())
    }

    fn complete(self, mut cx: TaskContext, _result: Result<(), String>) -> JsResult<JsUndefined> {
        Ok(cx.undefined())
    }
}

fn hello(mut cx: FunctionContext) -> JsResult<JsUndefined> {
    let f = cx.argument::<JsFunction>(0)?;
    BackgroundTask.schedule(f);
    Ok(cx.undefined())
}

register_module!(mut cx, {
    cx.export_function("hello", hello)
});

and edit lib/index.js to contain:

require('domain');
var addon = require('../native');

console.log(addon.hello(() => {}));

and then run it with node -e "require('.')". Note: node . works fine; just the REPL or node -e causes the failure mode. Likewise, if you comment out the requiring of domain, it works when run either way. Sync functions are not affected, and the body of the async tasks runs; failure seems not to occur until we try to call the JS-side callback function.

Other reports about the domain.enter is not a function seem to suggest that it's an issue that comes up when running Node 10 in combination with native addons built with NAN versions older than 2.10.0, but neon currently uses 2.10.0, and #410 (which updates to a newer NAN) seems not to fix the issue.

@kjvalencik
Copy link
Member

While working on #410, I noticed that there are warnings about deprecated MakeCallback usage. They need to be updated to support domains. That might be the cause.

@Zireael-N
Copy link
Contributor

Zireael-N commented Mar 13, 2020

What @apendleton did not mention is that this error causes the process to crash (happens with node v13.10.1 on macOS, v10.19.0 on macOS and v12.16.1 on Linux musl-libc).

Worth adding, that for me this happens in REPL without require('domain').

That might be the cause.

I can confirm that this is the cause. These changes in crates/neon-sys/native/src/neon_task.h:

    v8::Local<v8::Function> callback = v8::Local<v8::Function>::New(isolate_, callback_);
+   Nan::AsyncResource resource("nan:neon-task");
+   resource.runInAsyncScope(context->Global(), callback, 2, argv);
-   node::MakeCallback(isolate_, context->Global(), callback, 2, argv);
    callback_.Reset();
    context_.Reset();

fixed the issue for me. I did not make a PR because isolate_ is no longer used there and I'm not aware of the consequences as I'm not experienced with V8's internals.

@dherman, pinging you because this is related to #300.

@abbshr
Copy link

abbshr commented Aug 1, 2020

Does the problem has been fixed? It seems it still exist when running Node as REPL mode.

Test env:

  • os: macOS
  • node: v14.6.0

@Zireael-N
Copy link
Contributor

It has been fixed, but there were no new releases since then.

Here's a temporary solution from my Cargo.toml:

[build-dependencies]
neon-build = { git = "https://github.com/neon-bindings/neon.git", rev = "20df99bedc13ee1bd7f4801a74920bd81ff099d7" }

[dependencies]
neon = { git = "https://github.com/neon-bindings/neon.git", rev = "20df99bedc13ee1bd7f4801a74920bd81ff099d7" }

@kjvalencik
Copy link
Member

🤦 Sorry about that! We'll get a release out this week.

@That3Percent
Copy link

🤦 Sorry about that! We'll get a release out this week.

Looks like the release has slipped?

@kjvalencik
Copy link
Member

@That3Percent published.

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

Successfully merging a pull request may close this issue.

5 participants