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

src: improve embedder API #30467

Closed
wants to merge 28 commits into from
Closed
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
b6738bc
src: make `FreeEnvironment()` perform all necessary cleanup
addaleax Nov 10, 2019
7e2da59
src: fix memory leak in CreateEnvironment when bootstrap fails
addaleax Nov 11, 2019
8a04c59
src: move worker_context from Environment to IsolateData
addaleax Nov 12, 2019
b732a01
src: associate is_main_thread() with worker_context()
addaleax Nov 12, 2019
162b9ee
src: align worker and main thread code with embedder API
addaleax Nov 11, 2019
2f7cda2
fixup! src: align worker and main thread code with embedder API
addaleax Mar 2, 2020
eb04cac
fixup! src: align worker and main thread code with embedder API
addaleax Mar 12, 2020
cf89ff0
fixup! src: align worker and main thread code with embedder API
addaleax Mar 12, 2020
7336b7f
src: provide a variant of LoadEnvironment taking a callback
addaleax Nov 13, 2019
61098fc
src: add LoadEnvironment() variant taking a string
addaleax Nov 19, 2019
541d7f6
fixup! src: add LoadEnvironment() variant taking a string
addaleax Mar 12, 2020
c0b957c
test: re-enable cctest that was commented out
addaleax Feb 27, 2020
9bd88e7
src: add unique_ptr equivalent of CreatePlatform
addaleax Feb 28, 2020
99912a0
src: make InitializeNodeWithArgs() official public API
addaleax Feb 28, 2020
23b9ace
src: add ability to look up platform based on `Environment*`
addaleax Feb 28, 2020
ededfc9
src: allow non-Node.js TracingControllers
addaleax Feb 28, 2020
36ec54b
src: fix what a dispose without checking
Jan 8, 2020
718f91f
fixup! src: fix what a dispose without checking
addaleax Feb 28, 2020
12bb53a
src: shutdown platform from FreePlatform()
addaleax Feb 28, 2020
487df31
src,test: add full-featured embedder API test
addaleax Feb 28, 2020
799c9b8
fixup! src,test: add full-featured embedder API test
addaleax Feb 28, 2020
ab5e211
doc: add basic embedding example documentation
addaleax Feb 28, 2020
682e804
test: add extended embedder cctest
addaleax Mar 12, 2020
f191a72
fixup! doc: add basic embedding example documentation
addaleax Mar 16, 2020
3ce279a
fixup! src,test: add full-featured embedder API test
addaleax Mar 16, 2020
f46c44c
fixup! fixup! doc: add basic embedding example documentation
addaleax Mar 16, 2020
330f4a7
squash! test: add extended embedder cctest
addaleax Mar 17, 2020
634a289
fixup! squash! test: add extended embedder cctest
addaleax Mar 17, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 8 additions & 2 deletions Makefile
Expand Up @@ -212,6 +212,8 @@ coverage-clean:
$(RM) out/$(BUILDTYPE)/obj.target/node/src/tracing/*.gcno
$(RM) out/$(BUILDTYPE)/obj.target/cctest/src/*.gcno
$(RM) out/$(BUILDTYPE)/obj.target/cctest/test/cctest/*.gcno
$(RM) out/$(BUILDTYPE)/obj.target/embedtest/src/*.gcno
$(RM) out/$(BUILDTYPE)/obj.target/embedtest/test/embedding/*.gcno

.PHONY: coverage
# Build and test with code coverage reporting. Leave the lib directory
Expand Down Expand Up @@ -250,8 +252,8 @@ coverage-test: coverage-build
TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) $(COVTESTS)
$(MAKE) coverage-report-js
-(cd out && "../gcovr/scripts/gcovr" \
--gcov-exclude='.*\b(deps|usr|out|cctest)\b' -v -r Release/obj.target \
--html --html-detail -o ../coverage/cxxcoverage.html \
--gcov-exclude='.*\b(deps|usr|out|cctest|embedding)\b' -v \
-r Release/obj.target --html --html-detail -o ../coverage/cxxcoverage.html \
--gcov-executable="$(GCOV)")
@echo -n "Javascript coverage %: "
@grep -B1 Lines coverage/index.html | head -n1 \
Expand All @@ -276,6 +278,7 @@ coverage-report-js:
# Runs the C++ tests using the built `cctest` executable.
cctest: all
@out/$(BUILDTYPE)/$@ --gtest_filter=$(GTEST_FILTER)
@out/$(BUILDTYPE)/embedtest "require('./test/embedding/test.js')"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently bypassing the tools/test.py machinery, so it does not play along too well with the CI which relies on tap output - is there a reason? (I think it should work if you just use the addon test config?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should work if you just use the addon test config?

@joyeecheung I’m not sure what this refers to … this test is run very differently from the addons?

This is currently bypassing the tools/test.py machinery, so it does not play along too well with the CI which relies on tap output

So do the other cctests, too, if I’m not mistaken?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the CI we use gtest's XML output for cctest (gtest dropped tap support some time ago so we just upload the xml to Jenkins at the end of the test jobs):

node/Makefile

Line 531 in f46c44c

out/Release/cctest --gtest_output=xml:out/junit/cctest.xml

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@richardlau Any suggestions here? I don’t think we can really implement full TAP support for this test, but if it’s about failing/passing, I guess we could wrap the call to the embedding test in a shell script … I don’t really know what kind of integration that would require

(That being said, I don’t expect this test to be particularly platform-specific, so it’s most likely going to fail locally before we run into trouble in CI)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure what this refers to … this test is run very differently from the addons?

The addon tests work by:

  1. Building a binary
  2. Load that binary in a JS test

I think it's not too different from what's being done here other than that in this case the binary is an executable instead of a shared object (if you are using node.gyp to build it the dependency is handled just fine without extra modification), and we need to load the binary by spawning a process instead of dlopen, both are controllable via configs / source code.

If you use AddonTestConfiguration in the test/folder/testcfg.py, it would scan all files ending in .js in folder and run it with out/${BUILDTYPE}/node(.exe) with appropriate command line flags, whereas if you use SimpleTestCase it needs the tests to be named like r'^test-.*\.m?js$', either is fine, depending on how you want the directory structure to be. Of course the ideal solution would be to add another config in test/__init__.py and override e.g. GetCommand() to locate out/${BUILDTYPE}/embedtest(.exe) and run the test directly to save an extra indirection, but that's not a big deal (although if you already need that logic in a JS file, it's probably not that different to just do it in python)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(BTW I just noticed that the test is not run on Windows if you just use Makefile to run it)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung I understand how this would work conceptually, but that doesn’t mean that I know the concrete steps that would be necessary to implement this; in particular:

if you are using node.gyp to build it the dependency is handled just fine without extra modification

That would mean rebuilding all of Node.js, wouldn’t it?

Either way, if there’s an easy solution, I’m happy to see that implemented, but if this is a blocker for you then I’d prefer opening a separate PR for the test so that this PR can be merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean rebuilding all of Node.js, wouldn’t it?

I am talking about what's currently done in the PR is fine (i.e. you don't need extra config in vcbuild.bat to handle the dependency)

This should work though I have not tested it on Windows

diff --git a/test/embedding/test-embedding.js b/test/embedding/test-embedding.js
new file mode 100644
index 0000000000..6378215ca5
--- /dev/null
+++ b/test/embedding/test-embedding.js
@@ -0,0 +1,25 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const child_process = require('child_process');
+const path = require('path');
+
+common.allowGlobals(global.require);
+let binary = process.features.debug ? `out/Debug/embedtest` : `out/Release/embedtest`;
+if (common.isWindows) {
+  binary += '.exe';
+}
+binary = path.resolve(__dirname, '..', '..', binary);
+
+assert.strictEqual(
+  child_process.spawnSync(binary, ['console.log(42)'])
+    .stdout.toString().trim(),
+  '42');
+
+assert.strictEqual(
+  child_process.spawnSync(binary, ['throw new Error()']).status,
+  1);
+
+assert.strictEqual(
+  child_process.spawnSync(binary, ['process.exitCode = 8']).status,
+  8);
diff --git a/test/embedding/test.js b/test/embedding/test.js
deleted file mode 100644
index a802de1849..0000000000
--- a/test/embedding/test.js
+++ /dev/null
@@ -1,19 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-const child_process = require('child_process');
-
-common.allowGlobals(global.require);
-
-assert.strictEqual(
-  child_process.spawnSync(process.execPath, ['console.log(42)'])
-    .stdout.toString().trim(),
-  '42');
-
-assert.strictEqual(
-  child_process.spawnSync(process.execPath, ['throw new Error()']).status,
-  1);
-
-assert.strictEqual(
-  child_process.spawnSync(process.execPath, ['process.exitCode = 8']).status,
-  8);
diff --git a/test/embedding/testcfg.py b/test/embedding/testcfg.py
new file mode 100644
index 0000000000..2c2929f610
--- /dev/null
+++ b/test/embedding/testcfg.py
@@ -0,0 +1,6 @@
+import sys, os
+sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
+import testpy
+
+def GetConfiguration(context, root):
+  return testpy.SimpleTestConfiguration(context, root, 'embedding')
diff --git a/tools/test.py b/tools/test.py
index 112d5edaff..5eb1fa7672 100755
--- a/tools/test.py
+++ b/tools/test.py
@@ -1491,6 +1491,7 @@ IGNORED_SUITES = [
   'addons',
   'benchmark',
   'doctool',
+  'embedding',
   'internet',
   'js-native-api',
   'node-api',

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note: you'd need to add an extra case e.g. read from fixtures, or just a plain string, for testing the module system as in the diff above the file is directly run with the node binary and only cases inside are run with the embedtest binary)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung Done, added your changes + fixed up Makefile + an extra test that reads from fixtures in 330f4a7 – I’ll kick off CI and we’ll see how that goes.


.PHONY: list-gtests
list-gtests:
Expand Down Expand Up @@ -529,6 +532,7 @@ test-ci: | clear-stalled build-addons build-js-native-api-tests build-node-api-t
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC)
out/Release/embedtest 'require("./test/embedding/test.js")'
@echo "Clean up any leftover processes, error if found."
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand Down Expand Up @@ -1258,6 +1262,8 @@ LINT_CPP_FILES = $(filter-out $(LINT_CPP_EXCLUDE), $(wildcard \
test/addons/*/*.h \
test/cctest/*.cc \
test/cctest/*.h \
test/embedding/*.cc \
test/embedding/*.h \
test/js-native-api/*/*.cc \
test/js-native-api/*/*.h \
test/node-api/*/*.cc \
Expand Down
226 changes: 226 additions & 0 deletions doc/api/embedding.md
@@ -0,0 +1,226 @@
# C++ Embedder API

<!--introduced_in=REPLACEME-->

Node.js provides a number of C++ APIs that can be used to execute JavaScript
in a Node.js environment from other C++ software.

The documentation for these APIs can be found in [src/node.h][] in the Node.js
source tree. In addition to the APIs exposed by Node.js, some required concepts
are provided by the V8 embedder API.

Because using Node.js as an embedded library is different from writing code
that is executed by Node.js, breaking changes do not follow typical Node.js
[deprecation policy][] and may occur on each semver-major release without prior
warning.
addaleax marked this conversation as resolved.
Show resolved Hide resolved

## Example embedding application

The following sections will provide an overview over how to use these APIs
to create an application from scratch that will perform the equivalent of
`node -e <code>`, i.e. that will take a piece of JavaScript and run it in
a Node.js-specific environment.

The full code can be found [in the Node.js source tree][embedtest.cc].

### Setting up per-process state

Node.js requires some per-process state management in order to run:

* Arguments parsing for Node.js [CLI options][],
* V8 per-process requirements, such as a `v8::Platform` instance.

The following example shows how these can be set up. Some class names are from
the `node` and `v8` C++ namespaces, respectively.

```c++
int main(int argc, char** argv) {
std::vector<std::string> args(argv, argv + argc);
std::vector<std::string> exec_args;
std::vector<std::string> errors;
// Parse Node.js CLI options, and print any errors that have occurred while
// trying to parse them.
int exit_code = node::InitializeNodeWithArgs(&args, &exec_args, &errors);
for (const std::string& error : errors)
fprintf(stderr, "%s: %s\n", args[0].c_str(), error.c_str());
if (exit_code != 0) {
return exit_code;
}

// Create a v8::Platform instance. `MultiIsolatePlatform::Create()` is a way
// to create a v8::Platform instance that Node.js can use when creating
// Worker threads. When no `MultiIsolatePlatform` instance is present,
gireeshpunathil marked this conversation as resolved.
Show resolved Hide resolved
// Worker threads are disabled.
gireeshpunathil marked this conversation as resolved.
Show resolved Hide resolved
std::unique_ptr<MultiIsolatePlatform> platform =
MultiIsolatePlatform::Create(4);
V8::InitializePlatform(platform.get());
V8::Initialize();

// See below for the contents of this function.
int ret = RunNodeInstance(platform.get(), args, exec_args);

V8::Dispose();
V8::ShutdownPlatform();
return ret;
}
```

### Per-instance state

Node.js has a concept of a “Node.js instance”, that is commonly being referred
to as `node::Environment`. Each `node::Environment` is associated with:

* Exactly one `v8::Isolate`, i.e. one JS Engine instance,
* Exactly one `uv_loop_t`, i.e. one event loop, and
* A number of `v8::Context`s, but exactly one main `v8::Context`.
* One `node::IsolateData` instance that contains information that could be
shared by multiple `node::Environment`s that use the same `v8::Isolate`.
Currently, no testing if performed for this scenario.

In order to set up a `v8::Isolate`, an `v8::ArrayBuffer::Allocator` needs
to be provided. One possible choice is the default Node.js allocator, which
can be created through `node::ArrayBufferAllocator::Create()`. Using the Node.js
allocator allows minor performance optimizations when addons use the Node.js
C++ `Buffer` API, and is required in order to track `ArrayBuffer` memory in
[`process.memoryUsage()`][].

Additionally, each `v8::Isolate` that is used for a Node.js instance needs to
be registered and unregistered with the `MultiIsolatePlatform` instance, if one
is being used, in order for the platform to know which event loop to use
for tasks scheduled by the `v8::Isolate`.

The `node::NewIsolate()` helper function creates a `v8::Isolate`,
sets it up with some Node.js-specific hooks (e.g. the Node.js error handler),
and registers it with the platform automatically.

```c++
int RunNodeInstance(MultiIsolatePlatform* platform,
const std::vector<std::string>& args,
const std::vector<std::string>& exec_args) {
int exit_code = 0;
// Set up a libuv event loop.
uv_loop_t loop;
int ret = uv_loop_init(&loop);
if (ret != 0) {
fprintf(stderr, "%s: Failed to initialize loop: %s\n",
args[0].c_str(),
uv_err_name(ret));
return 1;
}

std::shared_ptr<ArrayBufferAllocator> allocator =
ArrayBufferAllocator::Create();

Isolate* isolate = NewIsolate(allocator, &loop, platform);
if (isolate == nullptr) {
fprintf(stderr, "%s: Failed to initialize V8 Isolate\n", args[0].c_str());
return 1;
}

{
Locker locker(isolate);
Isolate::Scope isolate_scope(isolate);

// Create a node::IsolateData instance that will later be released using
// node::FreeIsolateData().
std::unique_ptr<IsolateData, decltype(&node::FreeIsolateData)> isolate_data(
node::CreateIsolateData(isolate, &loop, platform, allocator.get()),
node::FreeIsolateData);

// Set up a new v8::Context.
HandleScope handle_scope(isolate);
Local<Context> context = node::NewContext(isolate);
if (context.IsEmpty()) {
fprintf(stderr, "%s: Failed to initialize V8 Context\n", args[0].c_str());
return 1;
}

// The v8::Context needs to be entered when node::CreateEnvironment() and
// node::LoadEnvironment() are being called.
Context::Scope context_scope(context);

// Create a node::Environment instance that will later be released using
// node::FreeEnvironment().
std::unique_ptr<Environment, decltype(&node::FreeEnvironment)> env(
node::CreateEnvironment(isolate_data.get(), context, args, exec_args),
node::FreeEnvironment);

// Set up the Node.js instance for execution, and run code inside of it.
// There is also a variant that takes a callback and provides it with
// the `require` and `process` objects, so that it can manually compile
// and run scripts as needed.
// The `require` function inside this script does *not* access the file
// system, and can only load built-in Node.js modules.
// `module.createRequire()` is being used to create one that is able to
// load files from the disk, and uses the standard CommonJS file loader
// instead of the internal-only `require` function.
MaybeLocal<Value> loadenv_ret = node::LoadEnvironment(
env.get(),
"const publicRequire ="
" require('module').createRequire(process.cwd() + '/');"
"globalThis.require = publicRequire;"
"require('vm').runInThisContext(process.argv[1]);");
gireeshpunathil marked this conversation as resolved.
Show resolved Hide resolved

if (loadenv_ret.IsEmpty()) // There has been a JS exception.
return 1;

{
// SealHandleScope protects against handle leaks from callbacks.
SealHandleScope seal(isolate);
bool more;
do {
uv_run(&loop, UV_RUN_DEFAULT);

// V8 tasks on background threads may end up scheduling new tasks in the
// foreground, which in turn can keep the event loop going. For example,
// WebAssembly.compile() may do so.
platform->DrainTasks(isolate);

// If there are new tasks, continue.
more = uv_loop_alive(&loop);
if (more) continue;

// node::EmitBeforeExit() is used to emit the 'beforeExit' event on
// the `process` object.
node::EmitBeforeExit(env.get());

// 'beforeExit' can also schedule new work that keeps the event loop
// running.
more = uv_loop_alive(&loop);
} while (more == true);
}

// node::EmitExit() returns the current exit code.
exit_code = node::EmitExit(env.get());

// node::Stop() can be used to explicitly stop the event loop and keep
// further JavaScript from running. It can be called from any thread,
// and will act like worker.terminate() if called from another thread.
node::Stop(env.get());
}

// Unregister the Isolate with the platform and add a listener that is called
// when the Platform is done cleaning up any state it had associated with
// the Isolate.
bool platform_finished = false;
platform->AddIsolateFinishedCallback(isolate, [](void* data) {
*static_cast<bool*>(data) = true;
}, &platform_finished);
platform->UnregisterIsolate(isolate);
isolate->Dispose();

// Wait until the platform has cleaned up all relevant resources.
while (!platform_finished)
uv_run(&loop, UV_RUN_ONCE);
int err = uv_loop_close(&loop);
assert(err == 0);

return exit_code;
}
```

[`process.memoryUsage()`]: process.html#process_process_memoryusage
[CLI options]: cli.html
[deprecation policy]: deprecations.html
[embedtest.cc]: https://github.com/nodejs/node/blob/master/test/embedding/embedtest.cc
[src/node.h]: https://github.com/nodejs/node/blob/master/src/node.h
1 change: 1 addition & 0 deletions doc/api/index.md
Expand Up @@ -15,6 +15,7 @@
* [Buffer](buffer.html)
* [C++ Addons](addons.html)
* [C/C++ Addons with N-API](n-api.html)
* [C++ Embedder API](embedding.html)
* [Child Processes](child_process.html)
* [Cluster](cluster.html)
* [Command Line Options](cli.html)
Expand Down
56 changes: 56 additions & 0 deletions node.gyp
Expand Up @@ -1217,6 +1217,62 @@
],
}, # cctest

{
'target_name': 'embedtest',
'type': 'executable',

'dependencies': [
'<(node_lib_target_name)',
'deps/histogram/histogram.gyp:histogram',
'deps/uvwasi/uvwasi.gyp:uvwasi',
'node_dtrace_header',
'node_dtrace_ustack',
'node_dtrace_provider',
],

'includes': [
'node.gypi'
],

'include_dirs': [
'src',
'tools/msvs/genfiles',
'deps/v8/include',
'deps/cares/include',
'deps/uv/include',
'deps/uvwasi/include',
'test/embedding',
],

'sources': [
'src/node_snapshot_stub.cc',
'src/node_code_cache_stub.cc',
'test/embedding/embedtest.cc',
],

'conditions': [
['OS=="solaris"', {
'ldflags': [ '-I<(SHARED_INTERMEDIATE_DIR)' ]
}],
# Skip cctest while building shared lib node for Windows
[ 'OS=="win" and node_shared=="true"', {
'type': 'none',
}],
[ 'node_shared=="true"', {
'xcode_settings': {
'OTHER_LDFLAGS': [ '-Wl,-rpath,@loader_path', ],
},
}],
['OS=="win"', {
'libraries': [
'Dbghelp.lib',
'winmm.lib',
'Ws2_32.lib',
],
}],
],
}, # embedtest

# TODO(joyeecheung): do not depend on node_lib,
# instead create a smaller static library node_lib_base that does
# just enough for node_native_module.cc and the cache builder to
Expand Down