From a1de45624059d089c3c20319356ded5380efedb4 Mon Sep 17 00:00:00 2001 From: Leroy Hopson Date: Thu, 23 Jun 2022 20:30:27 +0700 Subject: [PATCH] Prevent exit callback instance leaks De-references pty_baton's exit callback after it is called so it can be automatically released, preventing leaked instances. Adds basic implementation for Pipe's get_status() method and forces PTY to wait for child process to exit to ensure exit callback is cleaned up. Adds a test to check that exit callback is still called as usual. --- CHANGELOG.md | 2 ++ addons/godot_xterm/native/src/node_pty/unix/pty.cc | 6 ++++-- addons/godot_xterm/native/src/pipe.cpp | 11 ++++++++++- addons/godot_xterm/pty.gd | 3 ++- test/platform/unix/unix.test.gd | 6 ++++++ 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59138ce..d6a1fe1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `pipe.close()` method of the gdnative library is now registered. - Fixed 'Condition "!obj" is true.' error that would often print to console when closing terminals in the Terminal panel of the editor plugin. +- Fixed leaked instances that would occur when PTY exited but child process was still + running. ## [v2.0.0](https://github.com/lihop/godot-xterm/compare/v1.2.1...v2.0.0) - 2021-07-25 diff --git a/addons/godot_xterm/native/src/node_pty/unix/pty.cc b/addons/godot_xterm/native/src/node_pty/unix/pty.cc index dd1b64c..2f52ca0 100644 --- a/addons/godot_xterm/native/src/node_pty/unix/pty.cc +++ b/addons/godot_xterm/native/src/node_pty/unix/pty.cc @@ -445,8 +445,10 @@ static void pty_after_waitpid(uv_async_t *async) { Array argv = Array::make(baton->exit_code, baton->signal_code); - if (baton->cb != nullptr && baton->cb->is_valid()) + if (baton->cb != nullptr && baton->cb->is_valid()) { baton->cb->call_funcv(argv); + baton->cb = (Ref)nullptr; + } uv_close((uv_handle_t *)async, pty_after_close); } @@ -669,4 +671,4 @@ void PTYUnix::_register_methods() { register_method("process", &PTYUnix::process); } -void PTYUnix::_init() {} +void PTYUnix::_init() {} \ No newline at end of file diff --git a/addons/godot_xterm/native/src/pipe.cpp b/addons/godot_xterm/native/src/pipe.cpp index 164f7b8..d313c1d 100644 --- a/addons/godot_xterm/native/src/pipe.cpp +++ b/addons/godot_xterm/native/src/pipe.cpp @@ -25,6 +25,7 @@ void Pipe::_register_methods() { register_method("poll", &Pipe::_poll_connection); register_method("open", &Pipe::open); register_method("write", &Pipe::write); + register_method("get_status", &Pipe::get_status); register_method("close", &Pipe::close); register_signal("data_received", "data", @@ -56,11 +57,12 @@ godot_error Pipe::open(int fd, bool ipc = false) { RETURN_IF_UV_ERR( uv_read_start((uv_stream_t *)&handle, _alloc_buffer, _read_cb)); + status = 1; return GODOT_OK; } void Pipe::close() { - uv_close((uv_handle_t *)&handle, NULL); + uv_close((uv_handle_t *)&handle, _close_cb); uv_run(uv_default_loop(), UV_RUN_NOWAIT); } @@ -84,6 +86,8 @@ godot_error Pipe::write(String p_data) { } int Pipe::get_status() { + if (!uv_is_active((uv_handle_t *)&handle)) + status = 0; _poll_connection(); return status; } @@ -120,3 +124,8 @@ void _alloc_buffer(uv_handle_t *handle, size_t suggested_size, uv_buf_t *buf) { buf->base = (char *)malloc(suggested_size); buf->len = suggested_size; } + +void _close_cb(uv_handle_t *handle) { + Pipe *pipe = static_cast(handle->data); + pipe->status = 0; +} \ No newline at end of file diff --git a/addons/godot_xterm/pty.gd b/addons/godot_xterm/pty.gd index a895dbf..8b3a90f 100644 --- a/addons/godot_xterm/pty.gd +++ b/addons/godot_xterm/pty.gd @@ -96,9 +96,10 @@ func open(cols: int = DEFAULT_COLS, rows: int = DEFAULT_ROWS) -> Array: func _exit_tree(): - _exit_cb = null if _pid > 1: LibuvUtils.kill(_pid, Signal.SIGHUP) + while _pipe.get_status() != 0: + continue func _on_pipe_data_received(data): diff --git a/test/platform/unix/unix.test.gd b/test/platform/unix/unix.test.gd index 71500d6..ac9a7fe 100644 --- a/test/platform/unix/unix.test.gd +++ b/test/platform/unix/unix.test.gd @@ -73,6 +73,12 @@ func test_closes_pty_on_exit(): assert_eq(new_num_pts, num_pts) +func test_emits_exited_signal_when_child_process_exits(): + pty.call_deferred("fork", "exit") + yield(yield_to(pty, "exited", 1), YIELD) + assert_signal_emitted(pty, "exited") + + class Helper: static func _get_pts() -> Array: assert(false, "Abstract method")