Skip to content

Do not raise from close() on teardown paths - #12

Open
Borega wants to merge 2 commits into
Zvendson:mainfrom
Borega:fix/no-raise-during-teardown
Open

Do not raise from close() on teardown paths#12
Borega wants to merge 2 commits into
Zvendson:mainfrom
Borega:fix/no-raise-during-teardown

Conversation

@Borega

@Borega Borega commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Process.close() and Thread.close() raise Win32Exception when CloseHandle fails — and both are called from __del__.

Raising from a destructor is unraisable: Python prints Exception ignored in: ... and carries on. Worse, during interpreter shutdown module globals have already been set to None, so raise Win32Exception() degenerates into:

TypeError: exceptions must derive from BaseException

That message says nothing about the actual failure, cannot be caught at the call site, and appears at a point where the program is already exiting. In practice consumers end up string-matching it to get a clean shutdown.

Fix

close() gains raise_on_error: bool = True:

process.close()                        # unchanged: raises on failure
process.close(raise_on_error=False)    # returns False instead

__del__ uses the non-raising form. Explicit calls keep their existing contract, so nothing changes for code that checks the return value or catches Win32Exception.

Two related cleanups:

  • The handle reference is now dropped whether or not CloseHandle succeeds. Retrying a handle that will not close only repeats the failure, and holding the attribute keeps a dead object alive.
  • Process.close() returns True when there is no handle rather than calling CloseHandle(0), making it idempotent (Thread.close() already did this).

Tests

Adds tests/test_teardown.py (7 tests), including a regression test that runs a subprocess which leaves a Process and an open Thread alive until interpreter shutdown, then asserts stderr contains neither must derive from BaseException nor Exception ignored.

Both close() variants are covered in raising and non-raising mode, and a destructor test installs sys.unraisablehook to prove __del__ stays silent when CloseHandle fails.

Full suite: 81 passed, 2 skipped.

Copilot AI lite review requested due to automatic review settings August 4, 2026 13:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR prevents noisy/unhelpful exceptions during object teardown by making Process.close() / Thread.close() optionally non-raising, and using that mode from __del__ to avoid unraisable exceptions during interpreter shutdown.

Changes:

  • Add raise_on_error: bool = True to Process.close() and Thread.close(); teardown paths call close(raise_on_error=False).
  • Make Process.close() idempotent when there is no handle and drop handle references even when CloseHandle fails.
  • Add teardown-focused tests covering raising vs non-raising behavior and clean interpreter shutdown.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
tests/test_teardown.py Adds regression and unit tests for destructor/teardown behavior and close() semantics.
MemLib/Thread.py Adds raise_on_error option to Thread.close() and uses it from __del__.
MemLib/Process.py Adds raise_on_error option + idempotent close behavior and uses it from __del__.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread MemLib/Process.py Outdated
Comment on lines +236 to +240
Returns:
bool: True if the process was closed successfully, False otherwise.

Raises:
windows.Win32Exception: If the process handle could not be closed.
windows.Win32Exception: If the handle could not be closed and `raise_on_error`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 19a46eb -- now states that False is only returned when raise_on_error is False, and that True also covers the already-closed handle.

Comment thread MemLib/Thread.py Outdated
Comment on lines +202 to +206
Returns:
bool: True if the handle was closed or already closed, False on error.

Raises:
Win32Exception: If the handle could not be closed and `raise_on_error`.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 19a46eb, same wording as Process.close().

Comment thread tests/test_teardown.py Outdated
Comment on lines +94 to +96
result = subprocess.run([sys.executable, "-c", probe], capture_output=True)

stderr = result.stderr.decode()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 19a46eb -- now text=True with errors="replace", so a code-page mismatch can't turn an assertion failure into a UnicodeDecodeError.

Borega added 2 commits August 5, 2026 13:24
`Process.close()` and `Thread.close()` raise Win32Exception when CloseHandle
fails, and both are called from `__del__`. Raising from a destructor is
unraisable -- Python prints "Exception ignored in: ..." and continues -- and
during interpreter shutdown, when module globals have already been set to None,
`raise Win32Exception()` degenerates into:

    TypeError: exceptions must derive from BaseException

which reports nothing about the actual failure and is impossible to handle at
the call site. Consumers end up string-matching that message to survive
teardown.

`close()` gains `raise_on_error: bool = True`, so explicit calls keep raising
exactly as before while `__del__` uses `close(raise_on_error=False)`. The
handle reference is now dropped whether or not CloseHandle succeeds: retrying a
handle that will not close only repeats the failure, and holding the attribute
keeps a dead object alive.

`Process.close()` also returns True instead of calling CloseHandle(0) when
there is no handle, making it idempotent.

Adds tests/test_teardown.py (7 tests), including one that runs a subprocess to
completion and asserts stderr contains neither "must derive from
BaseException" nor "Exception ignored".
…ecode

Review feedback, three items:

- Process.close() and Thread.close() documented "False otherwise", which is unreachable
  with the default raise_on_error=True. Both now state that False is returned only when
  raise_on_error is False, and that True also covers the already-closed handle.
- The interpreter-shutdown probe decoded stderr with the default codec, which can fail on
  Windows depending on the active code page and would surface as a UnicodeDecodeError
  instead of the real assertion. Now uses text=True with errors="replace".
@Borega
Borega force-pushed the fix/no-raise-during-teardown branch from 19a46eb to 3014684 Compare August 5, 2026 11:26
@Borega

Borega commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto main — the conflict is resolved and this is mergeable again.

The conflict was with #9 (owns_handle), which landed in close() after this branch was cut. Both sides touched the same two spots; neither change was dropped:

Also folded in the Returns: clarification from the follow-up commit, since the two now interact: True means closed or already closed or borrowed-and-released; False only when raise_on_error=False and CloseHandle actually failed.

Full suite on the rebased branch: 184 passed, 2 skipped (the count grew because #5#11 merged in the meantime), including the 7 tests/test_teardown.py cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants