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.

Sign up for GitHub

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

Jump to bottom

src: add detailed embedder process initialization API #44121

Merged
merged 5 commits into from
Aug 5, 2022
Merged

src: add detailed embedder process initialization API #44121

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16c7a59
src: add detailed embedder process initialization API
addaleax Aug 3, 2022
e3cad18
fixup! src: add detailed embedder process initialization API
addaleax Aug 3, 2022
c933415
fixup! fixup! src: add detailed embedder process initialization API
addaleax Aug 3, 2022
46057ce
fixup! fixup! fixup! src: add detailed embedder process initializatio…
addaleax Aug 4, 2022
7973d3d
fixup! fixup! fixup! fixup! src: add detailed embedder process initia…
addaleax Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixup! src: add detailed embedder process initialization API
  • Loading branch information
@addaleax
addaleax committed Aug 3, 2022
commit e3cad1845c254618b25758031b0675bb34430e46
38 changes: 15 additions & 23 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,16 +646,12 @@ static void PlatformInit(ProcessInitializationFlags::Flags flags) {
// anything.
for (auto& s : stdio) {
const int fd = &s - stdio;
if (fstat(fd, &s.stat) == 0)
continue;
if (fstat(fd, &s.stat) == 0) continue;
// Anything but EBADF means something is seriously wrong. We don't
// have to special-case EINTR, fstat() is not interruptible.
if (errno != EBADF)
ABORT();
if (fd != open("/dev/null", O_RDWR))
ABORT();
if (fstat(fd, &s.stat) != 0)
ABORT();
if (errno != EBADF) ABORT();
if (fd != open("/dev/null", O_RDWR)) ABORT();
if (fstat(fd, &s.stat) != 0) ABORT();
}
}

Expand All @@ -679,16 +675,14 @@ static void PlatformInit(ProcessInitializationFlags::Flags flags) {
const int fd = &s - stdio;
int err;

do
s.flags = fcntl(fd, F_GETFL);
do s.flags = fcntl(fd, F_GETFL);
while (s.flags == -1 && errno == EINTR); // NOLINT
CHECK_NE(s.flags, -1);

if (uv_guess_handle(fd) != UV_TTY) continue;
s.isatty = true;

do
err = tcgetattr(fd, &s.termios);
do err = tcgetattr(fd, &s.termios);
while (err == -1 && errno == EINTR); // NOLINT
CHECK_EQ(err, 0);
}
Expand Down Expand Up @@ -754,19 +748,17 @@ static void PlatformInit(ProcessInitializationFlags::Flags flags) {
// Ignore _close result. If it fails or not depends on used Windows
// version. We will just check _open result.
_close(fd);
if (fd != _open("nul", _O_RDWR))
ABORT();
if (fd != _open("nul", _O_RDWR)) ABORT();
}
}
}
#endif // _WIN32
}


// Safe to call more than once and from signal handlers.
void ResetStdio() {
if (init_process_flags.load() &
ProcessInitializationFlags::kNoStdioInitialization) {
ProcessInitializationFlags::kNoStdioInitialization) {
return;
}

Expand Down Expand Up @@ -1025,8 +1017,8 @@ int InitializeNodeWithArgs(std::vector<std::string>* argv,
}

std::unique_ptr<InitializationResult> InitializeOncePerProcess(
Copy link
Contributor

@RaisinTen RaisinTen Aug 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I fully understand why this is wrapping the InitializationResult in a unique_ptr now. Is it because of:

The return value of the function is made an abstract class,
rather than a struct, for easier API/ABI stability.

? Does it mean that we will be able to change the InitializationResult part into something else in the future if the need arises, without breaking ABI compatibility?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it because of:

The return value of the function is made an abstract class,
rather than a struct, for easier API/ABI stability.

?

Exactly 👍

Does it mean that we will be able to change the InitializationResult part into something else in the future if the need arises, without breaking ABI compatibility?

Well, there are always limitations, but we could add new fields or change the internal representation of existing fields without breaking ABI compatibility. (There’s also more extensive solutions to this, of course, e.g. how the CommonEnvironmentSetup class avoids virtual functions and instead uses an internal struct to define its properties. I think that would be a bit much here but I’m happy to move to that model if that’s preferred.)

Copy link
Member

Choose a reason for hiding this comment

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

Are there other places in the embedder API where we don't go as far as CommonEnvironmentSetup? If we want to get to ABI stabilty in the embedder API I guess the question would be why not do that when InitializationResult is being added to the externa API if its possible/not an unreasonable amount of work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there other places in the embedder API where we don't go as far as CommonEnvironmentSetup?

We have numerous virtual classes, yes.

why not do that when InitializationResult is being added to the externa API

It’s a bit easier this way, because the instance is accessed from a number of different places in our code. It doesn’t matter much, really – we can make changes while keeping ABI stability fairly easily either way.

I’ve added a small change in 7973d3d to make sure that there is no way to create instances of this class externally. That should rule out any theoretical concerns about userland subclasses (where virtual classes do indeed become a bit more difficult ABI-wise).

@mhdawson Please take another look.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the additional info.

const std::vector<std::string>& args,
ProcessInitializationFlags::Flags flags) {
const std::vector<std::string>& args,
ProcessInitializationFlags::Flags flags) {
auto result = std::make_unique<InitializationResultImpl>();
result->args_ = args;

Expand Down Expand Up @@ -1155,8 +1147,8 @@ std::unique_ptr<InitializationResult> InitializeOncePerProcess(
// not useful as an exit code at all.
result->exit_code_ = ERR_GET_REASON(ERR_peek_error());
joyeecheung marked this conversation as resolved.
Show resolved Hide resolved
result->early_return_ = true;
result->errors_.emplace_back(
"OpenSSL configuration error:\n" + GetOpenSSLErrorString());
result->errors_.emplace_back("OpenSSL configuration error:\n" +
GetOpenSSLErrorString());
return result;
}
#else // OPENSSL_VERSION_MAJOR < 3
Expand All @@ -1171,7 +1163,7 @@ std::unique_ptr<InitializationResult> InitializeOncePerProcess(
result->early_return_ = true;
result->errors_.emplace_back(
"OpenSSL error when trying to enable FIPS:\n" +
GetOpenSSLErrorString());
GetOpenSSLErrorString());
return result;
}

Expand Down Expand Up @@ -1326,8 +1318,8 @@ int Start(int argc, char** argv) {
// Hack around with the argv pointer. Used for process.title = "blah".
argv = uv_setup_args(argc, argv);

std::unique_ptr<InitializationResult> result = InitializeOncePerProcess(
std::vector<std::string>(argv, argv + argc));
std::unique_ptr<InitializationResult> result =
InitializeOncePerProcess(std::vector<std::string>(argv, argv + argc));
for (const std::string& error : result->errors()) {
FPrintF(stderr, "%s: %s\n", result->args().at(0), error);
}
Expand Down
25 changes: 10 additions & 15 deletions src/node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,15 +261,10 @@ enum Flags : uint64_t {
// a flags argument to the InitializeOncePerProcess() replacement
// function.
kLegacyInitializeNodeWithArgsBehavior =
kNoStdioInitialization |
kNoDefaultSignalHandling |
kNoInitializeV8 |
kNoInitializeNodeV8Platform |
kNoInitOpenSSL |
kNoParseGlobalDebugVariables |
kNoAdjustResourceLimits |
kNoUseLargePages |
kNoPrintHelpOrVersionOutput,
kNoStdioInitialization | kNoDefaultSignalHandling | kNoInitializeV8 |
kNoInitializeNodeV8Platform | kNoInitOpenSSL |
kNoParseGlobalDebugVariables | kNoAdjustResourceLimits |
kNoUseLargePages | kNoPrintHelpOrVersionOutput,
};
} // namespace ProcessFlags
// TODO(addaleax): Make this the canonical name, as it is more descriptive.
Expand Down Expand Up @@ -317,12 +312,12 @@ NODE_EXTERN int Stop(Environment* env);
// The subset is roughly equivalent to the one given by
// `ProcessInitializationFlags::kLegacyInitializeNodeWithArgsBehavior`.
NODE_DEPRECATED("Use InitializeOncePerProcess() instead",
NODE_EXTERN int InitializeNodeWithArgs(
std::vector<std::string>* argv,
std::vector<std::string>* exec_argv,
std::vector<std::string>* errors,
ProcessInitializationFlags::Flags flags =
ProcessInitializationFlags::kNoFlags));
NODE_EXTERN int InitializeNodeWithArgs(
std::vector<std::string>* argv,
std::vector<std::string>* exec_argv,
std::vector<std::string>* errors,
ProcessInitializationFlags::Flags flags =
ProcessInitializationFlags::kNoFlags));

// Set up per-process state needed to run Node.js. This will consume arguments
// from args, and return information about the initialization success,
Expand Down
12 changes: 6 additions & 6 deletions test/embedding/embedtest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,10 @@ int main(int argc, char** argv) {
argv = uv_setup_args(argc, argv);
std::vector<std::string> args(argv, argv + argc);
std::unique_ptr<node::InitializationResult> result =
node::InitializeOncePerProcess(args, {
node::ProcessInitializationFlags::kNoInitializeV8,
node::ProcessInitializationFlags::kNoInitializeNodeV8Platform
});
node::InitializeOncePerProcess(
args,
{node::ProcessInitializationFlags::kNoInitializeV8,
node::ProcessInitializationFlags::kNoInitializeNodeV8Platform});

for (const std::string& error : result->errors())
fprintf(stderr, "%s: %s\n", args[0].c_str(), error.c_str());
Expand All @@ -40,8 +40,8 @@ int main(int argc, char** argv) {
V8::InitializePlatform(platform.get());
V8::Initialize();

int ret = RunNodeInstance(
platform.get(), result->args(), result->exec_args());
int ret =
RunNodeInstance(platform.get(), result->args(), result->exec_args());

V8::Dispose();
V8::DisposePlatform();
Expand Down