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

Split OS::execute into two methods #44514

Merged
merged 1 commit into from
Jan 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 20 additions & 11 deletions core/core_bind.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -228,24 +228,32 @@ Error _OS::shell_open(String p_uri) {
return OS::get_singleton()->shell_open(p_uri);
}

int _OS::execute(const String &p_path, const Vector<String> &p_arguments, bool p_blocking, Array p_output, bool p_read_stderr) {
OS::ProcessID pid = -2;
int exitcode = 0;
int _OS::execute(const String &p_path, const Vector<String> &p_arguments, Array r_output, bool p_read_stderr) {
List<String> args;
for (int i = 0; i < p_arguments.size(); i++) {
args.push_back(p_arguments[i]);
}
String pipe;
Error err = OS::get_singleton()->execute(p_path, args, p_blocking, &pid, &pipe, &exitcode, p_read_stderr);
p_output.clear();
p_output.push_back(pipe);
int exitcode = 0;
Error err = OS::get_singleton()->execute(p_path, args, &pipe, &exitcode, p_read_stderr);
r_output.push_back(pipe);
if (err != OK) {
return -1;
}
return exitcode;
}

int _OS::create_process(const String &p_path, const Vector<String> &p_arguments) {
List<String> args;
for (int i = 0; i < p_arguments.size(); i++) {
args.push_back(p_arguments[i]);
}
OS::ProcessID pid = 0;
Error err = OS::get_singleton()->create_process(p_path, args, &pid);
if (err != OK) {
return -1;
} else if (p_blocking) {
return exitcode;
} else {
return pid;
}
return pid;
}

Error _OS::kill(int p_pid) {
Expand Down Expand Up @@ -697,7 +705,8 @@ void _OS::_bind_methods() {
ClassDB::bind_method(D_METHOD("get_processor_count"), &_OS::get_processor_count);

ClassDB::bind_method(D_METHOD("get_executable_path"), &_OS::get_executable_path);
ClassDB::bind_method(D_METHOD("execute", "path", "arguments", "blocking", "output", "read_stderr"), &_OS::execute, DEFVAL(true), DEFVAL(Array()), DEFVAL(false));
ClassDB::bind_method(D_METHOD("execute", "path", "arguments", "output", "read_stderr"), &_OS::execute, DEFVAL(Array()), DEFVAL(false));
ClassDB::bind_method(D_METHOD("create_process", "path", "arguments"), &_OS::create_process);
ClassDB::bind_method(D_METHOD("kill", "pid"), &_OS::kill);
ClassDB::bind_method(D_METHOD("shell_open", "uri"), &_OS::shell_open);
ClassDB::bind_method(D_METHOD("get_process_id"), &_OS::get_process_id);
Expand Down
4 changes: 2 additions & 2 deletions core/core_bind.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,8 +155,8 @@ class _OS : public Object {
int get_low_processor_usage_mode_sleep_usec() const;

String get_executable_path() const;
int execute(const String &p_path, const Vector<String> &p_arguments, bool p_blocking = true, Array p_output = Array(), bool p_read_stderr = false);

int execute(const String &p_path, const Vector<String> &p_arguments, Array r_output = Array(), bool p_read_stderr = false);
int create_process(const String &p_path, const Vector<String> &p_arguments);
Error kill(int p_pid);
Error shell_open(String p_uri);

Expand Down
3 changes: 2 additions & 1 deletion core/os/os.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ class OS {
virtual int get_low_processor_usage_mode_sleep_usec() const;

virtual String get_executable_path() const;
virtual Error execute(const String &p_path, const List<String> &p_arguments, bool p_blocking = true, ProcessID *r_child_id = nullptr, String *r_pipe = nullptr, int *r_exitcode = nullptr, bool read_stderr = false, Mutex *p_pipe_mutex = nullptr) = 0;
virtual Error execute(const String &p_path, const List<String> &p_arguments, String *r_pipe = nullptr, int *r_exitcode = nullptr, bool read_stderr = false, Mutex *p_pipe_mutex = nullptr) = 0;
virtual Error create_process(const String &p_path, const List<String> &p_arguments, ProcessID *r_child_id = nullptr) = 0;
virtual Error kill(const ProcessID &p_pid) = 0;
virtual int get_process_id() const;
virtual void vibrate_handheld(int p_duration_ms = 500);
Expand Down
59 changes: 34 additions & 25 deletions doc/classes/OS.xml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,29 @@
[b]Note:[/b] This method is implemented on Linux, macOS and Windows.
</description>
</method>
<method name="create_process">
<return type="int">
</return>
<argument index="0" name="path" type="String">
</argument>
<argument index="1" name="arguments" type="PackedStringArray">
</argument>
<description>
Creates a new process that runs independently of Godot. It will not terminate if Godot terminates. The file specified in [code]path[/code] must exist and be executable. Platform path resolution will be used. The [code]arguments[/code] are used in the given order and separated by a space.
If the process creation succeeds, the method will return the new process ID, which you can use to monitor the process (and potentially terminate it with [method kill]). If the process creation fails, the method will return [code]-1[/code].
For example, running another instance of the project:
[codeblocks]
[gdscript]
var pid = OS.create_process(OS.get_executable_path(), [])
[/gdscript]
[csharp]
var pid = OS.CreateProcess(OS.GetExecutablePath(), new string[] {});
[/csharp]
[/codeblocks]
See [method execute] if you wish to run an external command and retrieve the results.
[b]Note:[/b] This method is implemented on Android, iOS, Linux, macOS and Windows.
</description>
</method>
<method name="delay_msec" qualifiers="const">
<return type="void">
</return>
Expand Down Expand Up @@ -71,48 +94,34 @@
</argument>
<argument index="1" name="arguments" type="PackedStringArray">
</argument>
<argument index="2" name="blocking" type="bool" default="true">
</argument>
<argument index="3" name="output" type="Array" default="[ ]">
<argument index="2" name="output" type="Array" default="[ ]">
</argument>
<argument index="4" name="read_stderr" type="bool" default="false">
<argument index="3" name="read_stderr" type="bool" default="false">
</argument>
<description>
Execute the file at the given path with the arguments passed as an array of strings. Platform path resolution will take place. The resolved file must exist and be executable.
The arguments are used in the given order and separated by a space, so [code]OS.execute("ping", ["-w", "3", "godotengine.org"], false)[/code] will resolve to [code]ping -w 3 godotengine.org[/code] in the system's shell.
This method has slightly different behavior based on whether the [code]blocking[/code] mode is enabled.
If [code]blocking[/code] is [code]true[/code], the Godot thread will pause its execution while waiting for the process to terminate. The shell output of the process will be written to the [code]output[/code] array as a single string. When the process terminates, the Godot thread will resume execution.
If [code]blocking[/code] is [code]false[/code], the Godot thread will continue while the new process runs. It is not possible to retrieve the shell output in non-blocking mode, so [code]output[/code] will be empty.
The return value also depends on the blocking mode. When blocking, the method will return an exit code of the process. When non-blocking, the method returns a process ID, which you can use to monitor the process (and potentially terminate it with [method kill]). If the process forking (non-blocking) or opening (blocking) fails, the method will return [code]-1[/code] or another exit code.
Example of blocking mode and retrieving the shell output:
Executes a command. The file specified in [code]path[/code] must exist and be executable. Platform path resolution will be used. The [code]arguments[/code] are used in the given order and separated by a space. If an [code]output[/code] [Array] is provided, the complete shell output of the process will be appended as a single [String] element in [code]output[/code]. If [code]read_stderr[/code] is [code]true[/code], the output to the standard error stream will be included too.
If the command is successfully executed, the method will return the exit code of the command, or [code]-1[/code] if it fails.
[b]Note:[/b] The Godot thread will pause its execution until the executed command terminates. Use [Thread] to create a separate thread that will not pause the Godot thread, or use [method create_process] to create a completely independent process.
For example, to retrieve a list of the working directory's contents:
[codeblocks]
[gdscript]
var output = []
var exit_code = OS.execute("ls", ["-l", "/tmp"], true, output)
var exit_code = OS.execute("ls", ["-l", "/tmp"], output)
[/gdscript]
[csharp]
var output = new Godot.Collections.Array();
int exitCode = OS.Execute("ls", new string[] {"-l", "/tmp"}, true, output);
[/csharp]
[/codeblocks]
Example of non-blocking mode, running another instance of the project and storing its process ID:
[codeblocks]
[gdscript]
var pid = OS.execute(OS.get_executable_path(), [], false)
[/gdscript]
[csharp]
var pid = OS.Execute(OS.GetExecutablePath(), new string[] {}, false);
int exitCode = OS.Execute("ls", new string[] {"-l", "/tmp"}, output);
[/csharp]
[/codeblocks]
If you wish to access a shell built-in or perform a composite command, a platform-specific shell can be invoked. For example:
To execute a composite command, a platform-specific shell can be invoked. For example:
[codeblocks]
[gdscript]
var output = []
OS.execute("CMD.exe", ["/C", "cd %TEMP% &amp;&amp; dir"], true, output)
OS.execute("CMD.exe", ["/C", "cd %TEMP% &amp;&amp; dir"], output)
[/gdscript]
[csharp]
var output = new Godot.Collections.Array();
OS.Execute("CMD.exe", new string[] {"/C", "cd %TEMP% &amp;&amp; dir"}, true, output);
OS.Execute("CMD.exe", new string[] {"/C", "cd %TEMP% &amp;&amp; dir"}, output);
[/csharp]
[/codeblocks]
[b]Note:[/b] This method is implemented on Android, iOS, Linux, macOS and Windows.
Expand Down
92 changes: 50 additions & 42 deletions drivers/unix/os_unix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -254,31 +254,26 @@ uint64_t OS_Unix::get_ticks_usec() const {
return longtime;
}

Error OS_Unix::execute(const String &p_path, const List<String> &p_arguments, bool p_blocking, ProcessID *r_child_id, String *r_pipe, int *r_exitcode, bool read_stderr, Mutex *p_pipe_mutex) {
Error OS_Unix::execute(const String &p_path, const List<String> &p_arguments, String *r_pipe, int *r_exitcode, bool read_stderr, Mutex *p_pipe_mutex) {
#ifdef __EMSCRIPTEN__
// Don't compile this code at all to avoid undefined references.
// Actual virtual call goes to OS_JavaScript.
ERR_FAIL_V(ERR_BUG);
#else
if (p_blocking && r_pipe) {
String argss;
argss = "\"" + p_path + "\"";

if (r_pipe) {
String command = "\"" + p_path + "\"";
for (int i = 0; i < p_arguments.size(); i++) {
argss += String(" \"") + p_arguments[i] + "\"";
command += String(" \"") + p_arguments[i] + "\"";
}

if (read_stderr) {
argss += " 2>&1"; // Read stderr too
command += " 2>&1"; // Include stderr
} else {
argss += " 2>/dev/null"; //silence stderr
command += " 2>/dev/null"; // Silence stderr
}
FILE *f = popen(argss.utf8().get_data(), "r");

ERR_FAIL_COND_V_MSG(!f, ERR_CANT_OPEN, "Cannot pipe stream from process running with following arguments '" + argss + "'.");

FILE *f = popen(command.utf8().get_data(), "r");
ERR_FAIL_COND_V_MSG(!f, ERR_CANT_OPEN, "Cannot create pipe from command: " + command);
char buf[65535];

while (fgets(buf, 65535, f)) {
if (p_pipe_mutex) {
p_pipe_mutex->lock();
Expand All @@ -289,56 +284,69 @@ Error OS_Unix::execute(const String &p_path, const List<String> &p_arguments, bo
}
}
int rv = pclose(f);

if (r_exitcode) {
*r_exitcode = WEXITSTATUS(rv);
}

return OK;
}

pid_t pid = fork();
ERR_FAIL_COND_V(pid < 0, ERR_CANT_FORK);

if (pid == 0) {
// is child

if (!p_blocking) {
// For non blocking calls, create a new session-ID so parent won't wait for it.
// This ensures the process won't go zombie at end.
setsid();
}

Vector<CharString> cs;
cs.push_back(p_path.utf8());
for (int i = 0; i < p_arguments.size(); i++) {
cs.push_back(p_arguments[i].utf8());
}

// The child process
Vector<char *> args;
for (int i = 0; i < cs.size(); i++) {
args.push_back((char *)cs[i].get_data());
args.push_back((char *)p_path.utf8().get_data());
for (int i = 0; i < p_arguments.size(); i++) {
args.push_back((char *)p_arguments[i].utf8().get_data());
}
args.push_back(0);

execvp(p_path.utf8().get_data(), &args[0]);
// still alive? something failed..
fprintf(stderr, "**ERROR** OS_Unix::execute - Could not create child process while executing: %s\n", p_path.utf8().get_data());
raise(SIGKILL);
// The execvp() function only returns if an error occurs.
CRASH_NOW_MSG("Could not create child process: " + p_path);
Copy link
Member

Choose a reason for hiding this comment

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

CRASH_NOW_MSG will may cause unnecessary crash report generation and "Godot quit unexpectedly" dialog popping up on macOS, when an invalid executable is specified (reintroduce #17615 fixed in #41188).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CRASH_NOW_MSG() calls __builtin_trap(). Although __builtin_trap() is implementation dependent, it should raise a SIGTRAP (to generate a stack trace if there is one) and then raise SIGKILL.

Copy link
Member

Choose a reason for hiding this comment

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

It's not triggering "Godot quit unexpectedly", but, unlike SIGKILL generating a completely useless crash log twice (once it's generated by crash_handler_osx.mm and redirected to null, and second time by OS crash handler).

Click to expand crash log
Exception Type:        EXC_BAD_INSTRUCTION (SIGABRT)
Exception Codes:       0x0000000000000001, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
*** multi-threaded process forked ***
crashed on child side of fork pre-exec

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff2039f462 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff203cd610 pthread_kill + 263
2   libsystem_c.dylib             	0x00007fff20320720 abort + 120
3   godot.osx.tools.x86_64        	0x000000010068c71c handle_crash(int) + 2908 (crash_handler_osx.mm:153)
4   libsystem_platform.dylib      	0x00007fff20411d7d _sigtramp + 29
5   ???                           	0x00007ffeef56fd18 0 + 140732913876248
6   godot.osx.tools.x86_64        	0x00000001046010e4 _OS::create_process(String const&, Vector<String> const&) + 228 (core_bind.cpp:252)
7   godot.osx.tools.x86_64        	0x0000000104636aa6 void call_with_variant_args_ret_helper<__UnexistingClass, int, String const&, Vector<String> const&, 0ul, 1ul>(__UnexistingClass*, int (__UnexistingClass::*)(String const&, Vector<String> const&), Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul, 1ul>) + 262 (binder_common.h:562)
8   godot.osx.tools.x86_64        	0x000000010463696e void call_with_variant_args_ret_dv<__UnexistingClass, int, String const&, Vector<String> const&>(__UnexistingClass*, int (__UnexistingClass::*)(String const&, Vector<String> const&), Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) + 350 (binder_common.h:397)
9   godot.osx.tools.x86_64        	0x0000000104636704 MethodBindTR<int, String const&, Vector<String> const&>::call(Object*, Variant const**, int, Callable::CallError&) + 228 (method_bind.h:451)
10  godot.osx.tools.x86_64        	0x0000000104bd787e Object::call(StringName const&, Variant const**, int, Callable::CallError&) + 1038 (object.cpp:784)
11  godot.osx.tools.x86_64        	0x000000010493f14a Variant::call(StringName const&, Variant const**, int, Variant&, Callable::CallError&) + 746 (variant_call.cpp:611)
12  godot.osx.tools.x86_64        	0x00000001014c8a8f GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) + 35711 (gdscript_vm.cpp:1372)
13  godot.osx.tools.x86_64        	0x00000001013b3a04 GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) + 180 (gdscript.cpp:1555)
14  godot.osx.tools.x86_64        	0x0000000104c09cc1 ScriptInstance::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) + 273 (script_language.cpp:314)
15  godot.osx.tools.x86_64        	0x00000001030bd50d Node::_notification(int) + 4285 (node.cpp:147)
16  godot.osx.tools.x86_64        	0x0000000103027bfb Node::_notificationv(int, bool) + 155 (node.h:45)
17  godot.osx.tools.x86_64        	0x0000000103027aaf CanvasItem::_notificationv(int, bool) + 63 (canvas_item.h:164)
18  godot.osx.tools.x86_64        	0x00000001030279bf Control::_notificationv(int, bool) + 63 (control.h:47)
19  godot.osx.tools.x86_64        	0x0000000104bd4b2b Object::notification(int, bool) + 59 (object.cpp:793)
20  godot.osx.tools.x86_64        	0x00000001030beb58 Node::_propagate_ready() + 280 (node.cpp:188)
21  godot.osx.tools.x86_64        	0x00000001030beaee Node::_propagate_ready() + 174 (node.cpp:180)
22  godot.osx.tools.x86_64        	0x00000001030c4b37 Node::_set_tree(SceneTree*) + 167 (node.cpp:2561)
23  godot.osx.tools.x86_64        	0x00000001030fbf1d SceneTree::initialize() + 45 (scene_tree.cpp:395)
24  godot.osx.tools.x86_64        	0x000000010068ed22 OS_OSX::run() + 66 (os_osx.mm:315)
25  godot.osx.tools.x86_64        	0x00000001006c23f2 main + 1090 (godot_main_osx.mm:79)
26  libdyld.dylib                 	0x00007fff203e8621 start + 1

Nothing terrible, but it's a half minute of fully loaded CPU core and 70 KB of wasted disk space for each failed call, with no apparent benefits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also looks like it's raising a SIGABRT, so yes, it makes sense to use the previous approach.

}

if (p_blocking) {
int status;
waitpid(pid, &status, 0);
if (r_exitcode) {
*r_exitcode = WIFEXITED(status) ? WEXITSTATUS(status) : status;
}
int status;
waitpid(pid, &status, 0);
if (r_exitcode) {
*r_exitcode = WIFEXITED(status) ? WEXITSTATUS(status) : status;
}
return OK;
#endif
}

} else {
if (r_child_id) {
*r_child_id = pid;
Error OS_Unix::create_process(const String &p_path, const List<String> &p_arguments, ProcessID *r_child_id) {
#ifdef __EMSCRIPTEN__
// Don't compile this code at all to avoid undefined references.
// Actual virtual call goes to OS_JavaScript.
ERR_FAIL_V(ERR_BUG);
#else
pid_t pid = fork();
ERR_FAIL_COND_V(pid < 0, ERR_CANT_FORK);

if (pid == 0) {
// The new process
// Create a new session-ID so parent won't wait for it.
// This ensures the process won't go zombie at the end.
setsid();

Vector<char *> args;
args.push_back((char *)p_path.utf8().get_data());
for (int i = 0; i < p_arguments.size(); i++) {
args.push_back((char *)p_arguments[i].utf8().get_data());
}
args.push_back(0);

execvp(p_path.utf8().get_data(), &args[0]);
// The execvp() function only returns if an error occurs.
CRASH_NOW_MSG("Could not create child process: " + p_path);
}

if (r_child_id) {
*r_child_id = pid;
}
return OK;
#endif
}
Expand Down
3 changes: 2 additions & 1 deletion drivers/unix/os_unix.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ class OS_Unix : public OS {
virtual void delay_usec(uint32_t p_usec) const override;
virtual uint64_t get_ticks_usec() const override;

virtual Error execute(const String &p_path, const List<String> &p_arguments, bool p_blocking = true, ProcessID *r_child_id = nullptr, String *r_pipe = nullptr, int *r_exitcode = nullptr, bool read_stderr = false, Mutex *p_pipe_mutex = nullptr) override;
virtual Error execute(const String &p_path, const List<String> &p_arguments, String *r_pipe = nullptr, int *r_exitcode = nullptr, bool read_stderr = false, Mutex *p_pipe_mutex = nullptr) override;
virtual Error create_process(const String &p_path, const List<String> &p_arguments, ProcessID *r_child_id = nullptr) override;
virtual Error kill(const ProcessID &p_pid) override;
virtual int get_process_id() const override;

Expand Down
9 changes: 3 additions & 6 deletions editor/editor_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2855,8 +2855,7 @@ void EditorNode::_discard_changes(const String &p_str) {
args.push_back(exec.get_base_dir());
args.push_back("--project-manager");

OS::ProcessID pid = 0;
Error err = OS::get_singleton()->execute(exec, args, false, &pid);
Error err = OS::get_singleton()->create_process(exec, args);
ERR_FAIL_COND(err);
} break;
}
Expand Down Expand Up @@ -5139,9 +5138,7 @@ void EditorNode::_global_menu_new_window(const Variant &p_tag) {
List<String> args;
args.push_back("-p");
String exec = OS::get_singleton()->get_executable_path();

OS::ProcessID pid = 0;
OS::get_singleton()->execute(exec, args, false, &pid);
OS::get_singleton()->create_process(exec, args);
}
}

Expand Down Expand Up @@ -5467,7 +5464,7 @@ void EditorNode::_print_handler(void *p_this, const String &p_string, bool p_err

static void _execute_thread(void *p_ud) {
EditorNode::ExecuteThreadArgs *eta = (EditorNode::ExecuteThreadArgs *)p_ud;
Error err = OS::get_singleton()->execute(eta->path, eta->args, true, nullptr, &eta->output, &eta->exitcode, true, &eta->execute_output_mutex);
Error err = OS::get_singleton()->execute(eta->path, eta->args, &eta->output, &eta->exitcode, true, &eta->execute_output_mutex);
print_verbose("Thread exit status: " + itos(eta->exitcode));
if (err != OK) {
eta->exitcode = err;
Expand Down
2 changes: 1 addition & 1 deletion editor/editor_run.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ Error EditorRun::run(const String &p_scene, const String &p_custom_args, const L
int instances = EditorSettings::get_singleton()->get_project_metadata("debug_options", "run_debug_instances", 1);
for (int i = 0; i < instances; i++) {
OS::ProcessID pid = 0;
Error err = OS::get_singleton()->execute(exec, args, false, &pid);
Error err = OS::get_singleton()->create_process(exec, args, &pid);
ERR_FAIL_COND_V(err, err);
pids.push_back(pid);
}
Expand Down
2 changes: 1 addition & 1 deletion editor/plugins/script_editor_plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2198,7 +2198,7 @@ bool ScriptEditor::edit(const RES &p_resource, int p_line, int p_col, bool p_gra
args.push_back(script_path);
}

Error err = OS::get_singleton()->execute(path, args, false);
Error err = OS::get_singleton()->create_process(path, args);
if (err == OK) {
return false;
}
Expand Down
Loading