Skip to content

fix: __del__ early-exit teardown path leaks pipe-drainer threads and has inconsistent GC-shutdown exception handling #4728

Description

@germa89

Context

Follow-up from the _kill_process/teardown refactor discussed on #4709. MapdlGrpc.__del__ was recently reworked so that instances which don't get a full _release_resources() teardown (already exited, cleanup_on_exit=False, _start_instance=False, or never _launched) still close the gRPC channel and mark themselves as exited, instead of leaking the channel/_poll_connectivity daemon thread entirely. Two gaps remain in that new early-exit branch:

1. PIPE-drainer threads are not joined

The analogous "leave MAPDL running" path used by exit()_disconnect_but_leave_mapdl_running() — closes the gRPC channel and joins _stdout_thread, _stderr_thread, and _startup_stdout_thread via _join_pipe_drainer_threads(). The new __del__ early-exit branch only closes the channel:

try:
    self._exiting = True
    self._close_grpc_channel()
except Exception as e:
    pass
finally:
    self._exited = True
    self._exiting = False
    return

For instances that hit this branch (for example _start_instance=False, connecting to an already-running/remote MAPDL, then letting the Python object be garbage-collected), the PIPE-drainer threads can leak.

Suggested fix: call self._disconnect_but_leave_mapdl_running() instead of hand-rolling _close_grpc_channel() + _exited = True, so both code paths share one implementation and stay in sync; or explicitly add a self._join_pipe_drainer_threads() call alongside the channel close.

2. Inconsistent/fragile exception handling during interpreter shutdown

_release_resources()'s teardown steps are each wrapped in try/except Exception: pass with an explicit # nosec B110 - best-effort cleanup during GC; logging is unreliable here rationale — deliberately not logging, because __del__ can run during interpreter shutdown when logging machinery (or self._log itself, on a partially-constructed instance) may already be gone.

A related but currently unused knob was added to _close_grpc_channel(self, exiting: bool = False) to suppress its internal self._log.debug(...) calls when exiting=True — presumably intended to apply the same "don't log during GC" rule here. However, none of the three call sites (_disconnect_but_leave_mapdl_running, _release_resources step 3, __del__'s early-exit branch) currently pass exiting=True, so the parameter has no effect yet, and __del__'s early-exit branch can still end up logging through _close_grpc_channel's internal self._log.debug calls on a partially-constructed instance during shutdown.

Suggested fix: either wire exiting=True into the __del__ (and possibly _disconnect_but_leave_mapdl_running) call sites so _close_grpc_channel suppresses its own logging there, or drop the parameter and keep the "silent pass, no logging" pattern already used elsewhere in _release_resources for consistency.

Suggested acceptance criteria

  • __del__'s early-exit branch does not leak _stdout_thread/_stderr_thread/_startup_stdout_thread.
  • Exception handling during __del__/GC teardown is consistent (either all paths log defensively, or none do), and does not risk raising a second unhandled exception from within an except block on a partially-constructed instance.
  • Add/extend unit tests (mock-based, no real MAPDL) covering both behaviors.

Discovered while reviewing #4709.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions