Split OS::execute into two methods #44514

madmiraal · 2020-12-19T10:22:21Z

Currently, OS::execute does two different things depending on whether the third parameter (blocking) is true or not. This PR splits OS::execute into two methods:

execute(): Executes a command and returns the results. The old default execute blocking = true method.
create_process(): Creates a new process and returns the new process' id. The old execute blocking = false method.

Note: It now also appends the shell output String to the provided output Array instead of overwriting it.

Closes #19302.
Part of #16863.

platform/javascript/os_javascript.cpp

1. execute(): Executes a command and returns the results. 2. create_process(): Creates a new process and returns the new process' id.

madmiraal · 2021-01-09T10:08:28Z

Rebased following merge of #44645.

Faless

LGTM, I don't have much experience with the Windows part, but the unix part seems good. Great job 🥇

Faless · 2021-01-12T15:04:57Z

tests/test_math.cpp

@@ -617,7 +617,7 @@ MainLoop *test() {

 	List<String> args;
 	args.push_back("-l");
-	Error err = OS::get_singleton()->execute("/bin/ls", args, true, nullptr, &ret);
+	Error err = OS::get_singleton()->execute("/bin/ls", args, &ret);


not sure why this is here, but it's fine for now should probably be re-evaluated in a separate PR.

That does look like a copy paste error from OS tests to math tests. Could be removed, but this can indeed be done in another PR. The whole file should be rewritten to use the new test framework anyway.

This should likely be listed in #43440 BTW @Calinou.

akien-mga · 2021-01-12T15:17:09Z

Thanks!

bruvzg · 2021-01-12T15:21:04Z

drivers/unix/os_unix.cpp

-		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);


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).

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.

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.

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

madmiraal added enhancement topic:core breaks compat labels Dec 19, 2020

madmiraal added this to the 4.0 milestone Dec 19, 2020

madmiraal requested review from hpvb, vnen and a team as code owners December 19, 2020 10:22

madmiraal mentioned this pull request Dec 19, 2020

[TRACKER] Methods, properties and signals to consider for renaming in next planned compatibility breakage #16863

Closed

83 tasks

akien-mga requested a review from Faless December 19, 2020 10:37

Faless reviewed Dec 20, 2020

View reviewed changes

platform/javascript/os_javascript.cpp Show resolved Hide resolved

madmiraal force-pushed the split-os-execute branch 2 times, most recently from e2b986e to 3159077 Compare December 21, 2020 11:53

akien-mga requested a review from Faless December 28, 2020 13:56

Split OS::execute into two methods

2a74b38

1. execute(): Executes a command and returns the results. 2. create_process(): Creates a new process and returns the new process' id.

madmiraal force-pushed the split-os-execute branch from 3159077 to 2a74b38 Compare January 9, 2021 10:03

Faless approved these changes Jan 12, 2021

View reviewed changes

akien-mga merged commit 1218441 into godotengine:master Jan 12, 2021

bruvzg reviewed Jan 12, 2021

View reviewed changes

madmiraal deleted the split-os-execute branch January 13, 2021 08:22

bruvzg mentioned this pull request Jan 13, 2021

[macOS] OS::create_process is not passing command line arguments correctly. #45153

Closed

madmiraal mentioned this pull request Jan 13, 2021

Raise SIGKILL instead of CRASH_NOW in child process when fork fails #45157

Merged

This was referenced Aug 30, 2021

Clicking "Online Docs" freezes the editor #51691

Closed

OS.execute without blocking mode causes zombie processes (defunct) #51366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split OS::execute into two methods #44514

Split OS::execute into two methods #44514

madmiraal commented Dec 19, 2020

madmiraal commented Jan 9, 2021

Faless left a comment

Faless Jan 12, 2021

akien-mga Jan 12, 2021 •

edited

Loading

akien-mga commented Jan 12, 2021

bruvzg Jan 12, 2021

madmiraal Jan 13, 2021

bruvzg Jan 13, 2021

madmiraal Jan 13, 2021

Split OS::execute into two methods #44514

Split OS::execute into two methods #44514

Conversation

madmiraal commented Dec 19, 2020

madmiraal commented Jan 9, 2021

Faless left a comment

Choose a reason for hiding this comment

Faless Jan 12, 2021

Choose a reason for hiding this comment

akien-mga Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Jan 12, 2021

bruvzg Jan 12, 2021

Choose a reason for hiding this comment

madmiraal Jan 13, 2021

Choose a reason for hiding this comment

bruvzg Jan 13, 2021

Choose a reason for hiding this comment

madmiraal Jan 13, 2021

Choose a reason for hiding this comment

akien-mga Jan 12, 2021 •

edited

Loading