Call the python reset from PythonIndicator.Reset - #9698
Conversation
There was a problem hiding this comment.
Hey @mkzung! Thanks, leaving a few comments
Fix works for the plain non-inheriting case, but two confirmed regressions: segfault via super().reset() in inheriting classes, and uncaught AttributeError from the HasAttr/GetPythonMethod name asymmetry. Resolving the reset method once in SetIndicator + a reentrancy guard would cover most findings — details inline.
| { | ||
| using (Py.GIL()) | ||
| { | ||
| _indicatorWrapper.GetMethod(nameof(Reset), pythonOnly: true)?.Invoke().Dispose(); |
There was a problem hiding this comment.
For inheriting classes _indicatorWrapper wraps the instance itself, so a subclass reset() calling super().reset() (the pre-PR correct pattern) recurses: C# Reset() → python reset() → CLR binding → C# Reset()... Reproduced on this branch: ~995 frames, fatal 0xC0000005. A reentrancy guard is needed here.
| public override void Reset() | ||
| { | ||
| // GetMethod throws when the attribute is absent, and returns null when it is CSharp | ||
| if (_indicatorWrapper != null && _indicatorWrapper.HasAttr(nameof(Reset))) |
There was a problem hiding this comment.
HasAttr is snake-case tolerant but GetPythonMethod falls back to PascalCase-only GetAttr("Reset") — so self.reset = False (non-method, no Reset) passes the guard then throws an uncaught AttributeError; worked before this PR. Also, a non-bound-method callable (self.Reset = lambda: ...) is silently skipped. Suggest resolving the reset method once in SetIndicator, like _pythonIsReadyProperty — also removes the per-call HasAttr GIL round-trip and avoids caching null under "Reset" in _pythonMethods (keyed by name only, ignores pythonOnly).
| _indicatorWrapper.GetMethod(nameof(Reset), pythonOnly: true)?.Invoke().Dispose(); | ||
| } | ||
| } | ||
| _isReady = false; |
There was a problem hiding this comment.
If the python reset() raises, _isReady = false / base.Reset() are skipped → half-reset indicator. Run them before the invoke or in a finally.
| indicator.Reset(); | ||
| indicator.Update(new IndicatorDataPoint(reference, 100m)); | ||
|
|
||
| Assert.AreEqual(100m, indicator.Current.Value); |
There was a problem hiding this comment.
Also assert IsReady is false (or Samples == 0) after Reset() — dropping _isReady = false wouldn't fail this test.
2 participants
Description
PythonIndicator never overrode Reset(), so a reset defined in python was never called and _isReady stayed set.
Related Issue
#9697
Motivation and Context
indicator_history resets before it replays, so it returned values mixed with whatever the indicator had already seen. Same class as #9686, #9687, #9688 and #9694, one level up.
Requires Documentation Change
No.
How Has This Been Tested?
Added ResetClearsTheStateHeldInPython to PythonIndicatorTests. It runs in all six python fixtures, both wrapping paths and both naming conventions, and fails on master at 100.75 against 100. The two duck-typed fixtures gained the reset they were missing.
Full suite,
--filter "TestCategory!=TravisExclude&TestCategory!=ResearchRegressionTests", run on pristine master and on this branch:Same three, and they fail on master as well: CommanCallback(Python), ThreadSafety, ZipBytesReturnsByteArrayWithCorrectLength. The extra six are the new test in its six fixtures.
One note on the guard: GetMethod throws when the attribute is missing altogether and returns null only when it resolves to C#, so an unguarded call breaks every python indicator that has no reset. The first version of this did exactly that, and only the full run caught it.
Types of changes
Checklist:
bug-<issue#>-<description>