Why is State.Terminated() run when another, different indicator is loaded?

[NinjaTrader_Matthew]

Thanks for the detailed questions. I am running this by development and will get back to you with our findings.

There was an issue identified where indicators which were removed from the configured list were not correctly finalized. This had downstream effects where that indicator would have reference to other objects such as ChartControl, and would then cause scripts to execute code based on logic in terminated in those scenarios. This is scheduled be fixed in the next release under NTEIGHT-9580 and I'm currently doing more testing to make sure this is handled correctly for other indicator scenarios (hosting indicators, superdom columns, market analyzer columns, etc).

I appreciate your persistence which ultimately helped narrow this problem down.

[NinjaTrader_Matthew]

The specific scenario we identified and will be resolved in the next build was:

1. Custom indicator already is on chart (instance #1)
2. Open indicator dialog -> instance #2 (clone of #1 above) and instance #3 (in the list of indicator names) are created + .SetDefaults is called -> good
3. Remove the Custom indicator from the list of “Configured” indicators and press OK

-> Only 2 instance .Terminated are called and not 3 -> This was a bug which did not finalize the indicator until the next time you open/closed the list, which I believe explains some of the delayed behavior as a sub note to your report.

However, so I do not mislead anyone into thinking otherwise: State.Terminated is designed to, and will continue to be called at the time any instance of an indicator is being terminated.

Why are members of an object being changed based on actions on another object? Specifically, why is the exit routine of a class being run because of actions outside the class: actions that have nothing to do with the first class?"

Per your scenarios, ChartControl is a global property and can be accessed by any other indicator, as well as other instances of the same indicator you're working with. In particular, the UI's "Configured" indicator instances was cloned from the running instance on the chart, so ChartControl would not have been null at the time the "Configured" instance is terminated from the UI. This caused an unrelated UI instance of the indicator to attempt to modify values which it did not modify to begin with.

So that results in your blue text i.e.,

BarMarginRightAtEntry: 0
CurrentBarMargin: 0

Since your private field defaults to 0, you checked only if ChartControl was not null and then tried to reset the value to a value that was still 0.

As such, you should take steps to make sure that the indicator instance was the actual indicator to modify a global property, and if so, only then undo those changes. In your example, you could check if that value was greater than 0 - or keep it agnostic of user preferences, it can more easily be accomplished simply by setting a flag during State.DataLoaded in your example, and then checking against that flag in terminated.

private bool shouldResetBarMargin = false;
private int		_intBarMarginRightAtEntry = 0;

protected override void OnStateChange()
{
	if (State == State.DataLoaded)
	{				
		if (ChartControl != null) 
		{
			this._intBarMarginRightAtEntry = ChartControl.Properties.BarMarginRight;
			ChartControl.Properties.BarMarginRight = Math.Max(this._intBarMarginRightAtEntry, 260);
			
			shouldResetBarMargin = true;
		}
	}
	
	else if (State == State.Terminated)
	{			
		if (shouldResetBarMargin)
		{
			if(ChartControl != null)
			{
				ChartControl.Properties.BarMarginRight = this._intBarMarginRightAtEntry;
									
				shouldResetBarMargin = false;
			}
				
		}				
	}
}

The above logic protects the indicator against every scenario which may generate cloned instances of the indicator. These are actions such as, but not limited to: accessing a copy on the ui, copy & pasting/dragging & dropping, duplicating to new charts/to new tabs, compiling, hosting indicators, using reflection to generate some instance for some add-on, etc).

Opening or closing a dialog box should not cause exit processing in anything unrelated to the target of the invocation of said databox. Most certainly, I cannot see why exit processing is run when I merely open a dialog and close it without doing anything.

Agreed, your indicator should not have been running its exit processing at this time, and this can be managed by changing the indicators terminated logic to only process exit code of which it actually needed to process.

I understand you may not feel the same and were comfortable with the NT7 approach which only terminated indicators after they were running on the chart, but after careful review, we conclude that setting State.Terminated and disposing of all if its resources is a best case approach to ensure the finalization of every instance of that indicator.

[koganam]

The specific scenario we identified and will be resolved in the next build was:

1. Custom indicator already is on chart (instance #1)
2. Open indicator dialog -> instance #2 (clone of #1 above) and instance #3 (in the list of indicator names) are created + .SetDefaults is called -> good
3. Remove the Custom indicator from the list of “Configured” indicators and press OK

-> Only 2 instance .Terminated are called and not 3 -> This was a bug which did not finalize the indicator until the next time you open/closed the list, which I believe explains some of the delayed behavior as a sub note to your report.

However, so I do not mislead anyone into thinking otherwise: State.Terminated is designed to, and will continue to be called at the time any instance of an indicator is being terminated.



Per your scenarios, ChartControl is a global property and can be accessed by any other indicator, as well as other instances of the same indicator you're working with. In particular, the UI's "Configured" indicator instances was cloned from the running instance on the chart, so ChartControl would not have been null at the time the "Configured" instance is terminated from the UI. This caused an unrelated UI instance of the indicator to attempt to modify values which it did not modify to begin with.

So that results in your blue text i.e.,

BarMarginRightAtEntry: 0
CurrentBarMargin: 0

Since your private field defaults to 0, you checked only if ChartControl was not null and then tried to reset the value to a value that was still 0.

As such, you should take steps to make sure that the indicator instance was the actual indicator to modify a global property, and if so, only then undo those changes. In your example, you could check if that value was greater than 0 - or keep it agnostic of user preferences, it can more easily be accomplished simply by setting a flag during State.DataLoaded in your example, and then checking against that flag in terminated.

private bool shouldResetBarMargin = false;
private int		_intBarMarginRightAtEntry = 0;

protected override void OnStateChange()
{
	if (State == State.DataLoaded)
	{				
		if (ChartControl != null) 
		{
			this._intBarMarginRightAtEntry = ChartControl.Properties.BarMarginRight;
			ChartControl.Properties.BarMarginRight = Math.Max(this._intBarMarginRightAtEntry, 260);
			
			shouldResetBarMargin = true;
		}
	}
	
	else if (State == State.Terminated)
	{			
		if (shouldResetBarMargin)
		{
			if(ChartControl != null)
			{
				ChartControl.Properties.BarMarginRight = this._intBarMarginRightAtEntry;
									
				shouldResetBarMargin = false;
			}
				
		}				
	}
}

The above logic protects the indicator against every scenario which may generate cloned instances of the indicator. These are actions such as, but not limited to: accessing a copy on the ui, copy & pasting/dragging & dropping, duplicating to new charts/to new tabs, compiling, hosting indicators, using reflection to generate some instance for some add-on, etc).




Agreed, your indicator should not have been running its exit processing at this time, and this can be managed by changing the indicators terminated logic to only process exit code of which it actually needed to process.

I understand you may not feel the same and were comfortable with the NT7 approach which only terminated indicators after they were running on the chart, but after careful review, we conclude that setting State.Terminated and disposing of all if its resources is a best case approach to ensure the finalization of every instance of that indicator.

Whereas I do not agree with your design decision, we must also acknowledge that it is your product, and you must be the final arbiter of what will be done.

The solution you propose is pretty much what Edge, NMJ and I, working together over this weekend, managed to figure out as what we considered to be a possible, kludgy, hacky, workaround.

Apparently, you collectively think that that is a solution, rather than a workaround. If that is the solution, even granted that maybe not many developers are using State.Terminated, there are still a considerable number of developers who understand the need to clean up in order to prevent resource leaks. As such, if this is the solution, then I shall have to request that it be officially documented. Further, it should actually be a part of the wizards, and so be placed as part of the skeleton whenever a strategy or indicator is built from the wizard.

In the absence of these, I shudder to think how many times your support is going to have to answer this question from developers who are not aware of this nuance. Is "nuance" even a valid description of this?

Just my $0.02.

[NinjaTrader_Matthew]

Whereas I do not agree with your design decision, we must also acknowledge that it is your product, and you must be the final arbiter of what will be done.

The solution you propose is pretty much what Edge, NMJ and I, working together over this weekend, managed to figure out as what we considered to be a possible, kludgy, hacky, workaround.

Apparently, you collectively think that that is a solution, rather than a workaround. If that is the solution, even granted that maybe not many developers are using State.Terminated, there are still a considerable number of developers who understand the need to clean up in order to prevent resource leaks. As such, if this is the solution, then I shall have to request that it be officially documented. Further, it should actually be a part of the wizards, and so be placed as part of the skeleton whenever a strategy or indicator is built from the wizard.

In the absence of these, I shudder to think how many times your support is going to have to answer this question from developers who are not aware of this nuance. Is "nuance" even a valid description of this?

Just my $0.02.

I agree, it should be documented and common practice that only a resource that has been set up/modified needs to be taken down/reset.

Anything we could do here Product/Feature wise would only plug a concept hole that a developer have not prepared themselves for, but we will do our best to make the documentation clearer here in this regard.

Thank you.

[koganam]

It should be documented and common practice that only a resource that has been set up/modified needs to be taken down/reset.

Which is exactly what I did. I modified something, then checked that it still existed before attempting to restore the object, only to get tripped up by something completely outside of the object that I was dealing with. So much so that you are now suggesting that I have to set up what amounts to another tracking system, to track what I have modified, instead of looking at the object itself to see if I modified it. If that is what you insist should be done, that is definitely beyond the pale of anything that is so-called "documented and common practice". I have a great deal of respect for your programming abilities, but that response strikes me as condescending and almost insulting.

Anything we could do here Product/Feature wise would only plug a concept hole that a developer have not prepared themselves for, but we will do our best to make the documentation clearer here in this regard. Thank you.

What "concept hole". That, in violation of the most basic concept of OOP, objects that are indicators or strategies in NinjaTrader can be modified by side-effects outside of the code in the object? That to me is a violation of OOP concepts. I guess maybe I do have a concept hole then, or maybe I, myself am the concept hole.

All I am asking is that you reduce your support load by telling everyone that you have coded this violation, and how to deal with it. In other words, warn people that if they do not take special measures, such as you have explained here, to track what has been modified, and not just look at the object itself, their cleanup code will run at times that they do not expect; as objects that have not been applied to a chart can attempt to be terminated, or objects that are already on the chart will attempt to run cleanup terminal code, if you attempt to react with the system by even preparing to load another object.

[NinjaTrader_Matthew]

What "concept hole". That, in violation of the most basic concept of OOP, objects that are indicators or strategies in NinjaTrader can be modified by side-effects outside of the code in the object? That to me is a violation of OOP concepts. I guess maybe I do have a concept hole then, or maybe I, myself am the concept hole.

All I am asking is that you reduce your support load by telling everyone that you have coded this violation, and how to deal with it. In other words, warn people that if they do not take special measures, such as you have explained here, to track what has been modified, and not just look at the object itself, their cleanup code will run at times that they do not expect; as objects that have not been applied to a chart can attempt to be terminated, or objects that are already on the chart will attempt to run cleanup terminal code, if you attempt to react with the system by even preparing to load another object.

To clarify, I mean if we were to provide some of bool or indication that the indicator has seen a certain state - but unless the developer knew to they needed to check for this themselves, they would still run into that situation.

For example, if we provided a HasSeenHistoricalState bool, it could definitely help as you would not need to implement this yourself - however you would still need to check for it through code and would still require a bit of education as to why it needs to be implemented, and the current proposed solution is fairly trivial for a developer to implement.

I have marked on our list with greater priority to work on improving the help documents around lifetime management of an indicator, from setup to termination, and hope to provide more sound documentation to make users aware of this design pattern and the solution proposed in my prior post.

We are not currently in any state where we can change the way indicators state changes, but will consider this scenario for future iterations of NinjaTrader 8.