Skip to content

Clear pending Python error when a Try-style conversion fails#138

Open
jhonabreul wants to merge 3 commits into
QuantConnect:masterfrom
jhonabreul:bug-tryas-leaves-pending-python-error
Open

Clear pending Python error when a Try-style conversion fails#138
jhonabreul wants to merge 3 commits into
QuantConnect:masterfrom
jhonabreul:bug-tryas-leaves-pending-python-error

Conversation

@jhonabreul

@jhonabreul jhonabreul commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

What does this implement/fix? Explain your changes.

PyObject.TryAsManagedObject (and therefore TryAs<T>/TrySafeAs<T>) converted with setError: true and returned false on failure, leaving the Python error indicator set on the thread with no one left to surface it. Callers of a Try* API only observe the boolean, so the stale error survived on the thread state and was thrown by the next unrelated Python call on that thread that checks the error indicator.

This was previously masked because the outermost Py.GIL scope deleted the thread state (and its pending error) on dispose. Since #137 pins thread states for the lifetime of the run, the leaked error persists and poisons subsequent calls on the same thread. This is the root cause of the PythonUtilTests.BothInheritedAndNonInheritedClassesWork(True) failure in QuantConnect/Lean#9614's CI: ExtensionsTests.PyObjectTryConvertToNonDelegateFail exercises a failed TryAs<int> on a Python function, leaving TypeError: int() argument must be a string, a bytes-like object or a real number, not 'function' pending on the NUnit worker thread, which PyModule.FromString in the next Python-touching test then throws. Reproduced deterministically by running both fixtures on a single NUnit worker; fixed with this change.

The fix: TryAsManagedObject now converts with setError: false and clears anything a conversion sub-path may have left pending before returning false. AsManagedObject keeps converting with setError: true so the fetched error still becomes the InvalidCastException cause (fetching consumes the indicator, so that path never leaked).

Also bumps the version to 2.0.63 (package <Version>, AssemblyVersion/AssemblyFileVersion, perf-test baseline reference).

A follow-up commit extends the same guarantee to the converter's collection and label paths, which could leak a pending error even with setError: false: ToList now handles a failed PyObject_GetIter like ToArray does (also fixing a leaked iterator reference); MakeList now fails the conversion when the iterator raises mid-iteration instead of returning a silently truncated list, and clears swallowed errors when not setting errors; and the type_error/overflow labels clear pending errors when not setting errors, mirroring convert_error. The unconditional raises in the implicit-conversion catches are kept on purpose — MethodBinder.Invoke surfaces a pending error as the binding failure reason (covered by ImplicitConversionErrorHandling).

Does this close any currently open issues?

Fixes the PythonUtilTests.BothInheritedAndNonInheritedClassesWork CI failure in QuantConnect/Lean#9614.

Any other comments?

Regression tests added:

  • RaisingIteratorFailsArrayConversion — a raising iterator fails the conversion in both setError modes, leaving an error pending only when setError is true.
  • OverflowConversionOnlyLeavesErrorWhenSettingErrors — overflowing numeric conversions leave an error pending only when setError is true.
  • FailedTryAsDoesNotLeavePythonErrorSet — a failed TryAs<int>/TryAsManagedObject on a Python function leaves no pending error and subsequent PyModule.FromString calls work.
  • FailedAsManagedObjectRaisesWithConversionErrorAsCauseAsManagedObject still raises InvalidCastException with the Python conversion error as its cause and no pending error afterwards.

Verified end-to-end against Lean: with 2.0.62, running Common.Util.ExtensionsTests + Common.Util.PythonUtilTests on a single NUnit worker reproduces the exact CI failure; with this build (2.0.63) all 352 tests pass. The full pythonnet embedding test suite passes (960 passed, 8 skipped). A full Lean test-suite sweep with a per-test PyErr-leak detector found this TryAs path to be the only leak source across 36,316 tests.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Ensure you have signed the .NET Foundation CLA
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

PyObject.TryAsManagedObject converted with setError: true and returned
false on failure, leaving the Python error indicator set on the thread
with no one left to surface it. Callers of TryAs/TryAsManagedObject
only observe the boolean, so the stale error survived on the thread
state and was thrown by the next unrelated Python call that checks the
error indicator.

This was previously masked because the outermost Py.GIL scope deleted
the thread state (and its pending error) on dispose; since thread
states are pinned for the lifetime of the run (QuantConnect#137), the leaked error
persists and poisons subsequent calls on the same thread - e.g. Lean's
PythonUtilTests failing with "int() argument must be a string, a
bytes-like object or a real number, not 'function'" leaked by an
earlier test that exercises a failed TryAs<int> on a function.

Convert with setError: false in TryAsManagedObject and clear any error
a conversion sub-path may have left pending before returning false.
AsManagedObject keeps converting with setError: true so the fetched
error still becomes the InvalidCastException cause, which also
consumes the indicator.
Bump package <Version>, AssemblyVersion/AssemblyFileVersion and the
perf-test baseline reference to 2.0.63.
Several Converter failure paths could leave a Python error pending even
when called with setError: false, relying on the caller to clean up:

- ToList did not handle a failed PyObject_GetIter: the raised TypeError
  survived the call (and the iterator reference leaked). Handle it like
  ToArray does, disposing the reference and clearing when not setting
  errors.
- MakeList treated a null PyIter_Next result as normal exhaustion, but
  null also means the iterator raised: the conversion now fails instead
  of returning a silently truncated list, clearing the error when not
  setting errors and leaving it pending for the caller when setting
  errors. The swallowed CLR-exception catch and the element-conversion
  failure path also clear when not setting errors, since some element
  conversion paths (failed implicit operators, deleted types) raise
  regardless of setError to enrich MethodBinder's no-match message.
- The type_error and overflow labels now clear pending errors when not
  setting errors, mirroring what convert_error already did, so a
  probing sub-path that raised before jumping cannot leak.

The unconditional raises in the implicit-conversion catches are kept:
MethodBinder.Invoke deliberately surfaces a pending error as the
binding failure reason instead of its generic no-match message, and
Converter.TryAsManagedObject clears at the API boundary.
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