[NIXL] Terminate handshake listener thread in shutdown

[markmc]

It turns out that we're not terminating the handshake listener thread during shutdown.

I discovered this while adding a (now unneeded) abort-after-finished variant of the abort_timeout_on_prefiller test and saw a port-in-use hang.

xref #26351

[gemini-code-assist]

Code Review

Thank you for this contribution. The changes correctly address the issue of the handshake listener thread not being terminated during shutdown. I've identified one high-severity issue related to the use of __del__ for resource cleanup, which is an unreliable practice in Python and can introduce thread-safety problems. Please see my detailed comment below.

[gemini-code-assist]

Using __del__ for resource cleanup is an anti-pattern in Python because its execution is not guaranteed. It may not be called if the object is part of a reference cycle, or during interpreter shutdown, which can lead to resource leaks that are hard to debug. The explicit shutdown() calls, like the one you've added in the tests, are the correct and reliable way to handle cleanup.

Furthermore, __del__ can be invoked by the garbage collector at any time and from any thread. This makes the thread-safety of shutdown() critical, but the current implementation is not thread-safe. For example:

• One thread could be iterating over self._recving_transfers while another clears it, causing a RuntimeError.
• self._nixl_handshake_listener_t could be set to None by one thread after another has checked it for None but before calling .join() on it, resulting in an AttributeError.

I strongly recommend removing the __del__ method and relying solely on explicit shutdown() calls. If shutdown() might be called concurrently from other paths, it should be protected with a threading.Lock.

[markmc]

Calling shutdown() from here is convenient for tests - it means resources are cleaned up even without explicit shutdown calls. Outside of tests, we should be calling shutdown() explicitly

Another example of us doing this:

vllm/v1/engine/async_llm.py:235:    def __del__(self):
vllm/v1/engine/async_llm.py-236-        self.shutdown()

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @markmc.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[NickLucche]

lgtm thanks @markmc , only left one comment.
Let's get ci green and merge

[NickLucche]

ok so this replaces the busy waiting mechanism without hanging on recv so we can quit, nice

[markmc]

FAILED entrypoints/openai/test_vision.py::test_single_chat_session_image_base64encoded_beamsearch[2-microsoft/Phi-3.5-vision-instruct] - AssertionError: assert 'The image sh... diagram with' == 'This image s...th three over'

Fixed by #26978, rebasing

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @markmc.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[markmc]

Rebased to pick up #27262