Execution sequence for my indicator -- need help

[koganam]

That depends on your definition of "instantiated". The constructor got called, and so presumably memory for he object has been allocated. What "instantiated" normally means, however, is that the constructor has returned. Calls to the instance's methods are not legal until that point.

--EV

We must be talking at cross-purposes. That is what I have trying to say. Your constructor has not completed creating your object, but the base.SetState() can be still called, and is. That is what is on the stack, because your object has not yet overridden the base event handler/method, which it will first inherit, then override.

[NinjaTrader_Brett]

Thanks for taking the time to write feedback on the design. I'll forward it to the development team.

To set expectations correctly, the architecture of NT8 is pretty well set in stone. We are in beta working to improve product stability to be able to make a release and we're not at a point in the development cycle where we could make architectural changes of this nature.I would suggest you start work to adjust any code that you normally would run in the constructor to work in State.SetDefaults appropriately since with NinjaTrader 8 this is how the indicator framework is setup and intended to be used.

[ETFVoyageur]

We must be talking at cross-purposes. That is what I have trying to say. Your constructor has not completed creating your object, but the base.SetState() can be still called, and is. That is what is on the stack, because your object has not yet overridden the base event handler/method, which it will first inherit, then override.

I have no problem with the NT constructor calling anything (of its own) that it wants to, including base.SetState(). I have a huge problem with that having the side effect of calling into code in my raw instance (its constructor has not run).

What do you think of my suggestion that they get rid of the SetDefaults call to OnStateChanged() and advocate setting defaults in the constructor? From the indicator's point of view getting to State.SetDefaults is not a state change -- it is the initial state when its constructor gets to run.

--EV

[ETFVoyageur]

Thanks for taking the time to write feedback on the design. I'll forward it to the development team.

To set expectations correctly, the architecture of NT8 is pretty well set in stone. We are in beta working to improve product stability to be able to make a release and we're not at a point in the development cycle where we could make architectural changes of this nature.I would suggest you start work to adjust any code that you normally would run in the constructor to work in State.SetDefaults appropriately since with NinjaTrader 8 this is how the indicator framework is setup and intended to be used.

"Intended to be used" does not include any provision for the object doing whatever initialization it wants to before setting defaults. That is an egregiously bad design. That said, and understanding your comment about the point in the release cycle, what about having the indicator

• Ignore the premature call to OnStateChanged() for State.SetDefaults
• Set any needed defaults in its constructor -- as you have explained, the state will be fine for that.

Is there some reason why this would not be a good way to satisfy both of us for now -- the need to get the product out the door (with which I heartily agree) and the indicator's need for some minor initialization prior to setting defaults.

--EV

[koganam]

I have no problem with the NT constructor calling anything (of its own) that it wants to, including base.SetState(). I have a huge problem with that having the side effect of calling into code in my raw instance (its constructor has not run).

What do you think of my suggestion that they get rid of the SetDefaults call to OnStateChanged() and advocate setting defaults in the constructor? From the indicator's point of view getting to State.SetDefaults is not a state change -- it is the initial state when its constructor gets to run.

--EV

So have you actually coded anything in your State.SetDefaults() and confirmed that that code is running code in the object, before the constructor returns? If it is actually running code before the constructor returns, yes, that would be very bad, design decision or not. Indeed to run anything in an object that has not yet been created borders on malware behavior.

On the other hand, if base.SetState() is being run merely to set the state to the required initial state, preparatory to your object constructor returning, so that the framework can get to work immediately after your constructor returns, then that sounds like a framework optimization. It would then appear that the framework is running SetState() to ensure things are kosher, and then putting the system into required initial state, even before the constructor returns. That way it can start SetDefaults()ing as soon as the constructor returns, because the call to OnStateChange() is now trivial and redundant, (and probably not even called again. I cannot be sure of that as I have not bothered to actually profile the process, so that is just speculation).

[NinjaTrader_Brett]

I don't see any blockers currently why that would not work for you to do. For that purpose you could basically ignore SetDefaults completely. Sounds like you could give that a try to head down the path you wanted to in the near term.

[ETFVoyageur]

So have you actually coded anything in your State.SetDefaults() and confirmed that that code is running before the constructor returns? If it is actually running code before the constructor returns, yes, that would be very bad, design decision or not.

Yes, I have. That's how I got into this discussion. I got unexpected behavior, which I traced to OnStateChanged(State.SetDefaults) getting called before my constructor had run.

On the other hand, if base.SetState() is being run merely to set the state to the required initial state, preparatory to your object constructor returning, so that the framework can get to work immediately after your constructor returns, then that sounds like a framework optimization. It would then appear that the framework is running SetState() to ensure things are kosher, and putting the system into required initial state, even before the constructor returns.

Agreed. I have no problem with NT running any of its own code it wants to. It is up to NT to avoid any bad side effects, though, such as calling the indicator's methods before the constructor has completed.

That way it can start SetDefaults()ing as soon as the constructor returns

What you describe is exactly what I keep saying it should be doing, but it is not -- it is jumping the gun and calling OnStateChanged(State.SetDefaults) before the constructor has returned -- twice (see below).

because the call to OnStateChange() is now trivial and redundant

The uselessness of the call is why I proposed that it be abandoned in favor of just setting defaults in the indicator's constructor, now that we know the state will be OK for that. The result would be a clean model, with minimal work on NT's part. It really should be dnoe NOW, though, because it would be a code-breaking change once NT8 is released.

(and probably not even called again. I cannot be sure of that as I have not bothered to actually profile the process, so that is just speculation).

I just continued a bit further than before and discovered that OnStateChanged() is called twice before the constructor returns. Both times the state is State.SetDefaults. Clearly the second one is a logical error -- both the old and the new state are the same, so there has been no state change. I was about to report that and ask what is going on when I saw your note. The undocumented consequence is that you had better not have any single-time code in your handler for State.SetDefaults, because it is going to get called at least twice.

There is a lot to like about NT8, but this specific thing is a botch. Taking Brett at his word that it won't be fixed this late in the Beta, I strongly suggest that the best way to have clean indicator code is to

• Ignore calls to OnStateChanged() when the state is State.SetDefaults
• Set your defaults from your constructor -- that will be after the state is State.SetDefaults, as Brett has confirmed and as I have confirmed with the debugger.

I really do hope they will fix the botch before release, though. I appreciate their time-to-market need, but waiting to fix it is also a problem because it could be a code-breaking change.

--EV

[ETFVoyageur]

I don't see any blockers currently why that would not work for you to do. For that purpose you could basically ignore SetDefaults completely. Sounds like you could give that a try to head down the path you wanted to in the near term.

Thanks for your reply. I'll give that a try. I do not see any downside.

I trust you noticed that Koganam now agrees with me

If it is actually running code before the constructor returns, yes, that would be very bad, design decision or not. Indeed to run anything in an object that has not yet been created borders on malware behavior.

It really is a bad design that needs to get fixed. The fix I suggest is very simple, but would break code -- better now than after release. There are fixes that should not break code, such as waiting until after the constructor returns before calling OnStateChanged(), but they would probbly be more work for NT developers.

BTW: what's up with OnStateChanged() getting called twice before the constructor has returned, both times with the same state (SetDefaults)? Clearly the second time is not a state change. You should at least fix that, or you risk a problem with a developer who cannot tolerate the redundant (erroneous?) call.

--EV

[NinjaTrader_Brett]

On the BTW comment. I'm not seeing that OnStateChanged getting called twice over here. Whats the scenario can you reproduce on your side with SMA indicator? If so whats different in your code that would help me narrow down to look into what is occurring?

[ETFVoyageur]

On the BTW comment. I'm not seeing that OnStateChanged getting called twice over here. Whats the scenario can you reproduce on your side with SMA indicator? If so whats different in your code that would help me narrow down to look into what is occurring?

Never mind -- a silly observational error. It is getting called only once. I regret saying otherwise.

--EV

[sledge]

So have you actually coded anything in your State.SetDefaults() and confirmed that that code is running code in the object, before the constructor returns? If it is actually running code before the constructor returns, yes, that would be very bad, design decision or not. Indeed to run anything in an object that has not yet been created borders on malware behavior.

I'm reading multiple things in here.

Constructor called but not completed with onstatechange running before constructor completion.
The other is/was the onstatechange called before the constructor to set defaults.

I will ask is this a multi threading issue if it is called before completion (of that is really happening).

Sorry if I'm really missing the boat here.

[ETFVoyageur]

I'm reading multiple things in here.

Constructor called but not completed with onstatechange running before constructor completion.
The other is/was the onstatechange called before the constructor to set defaults.

I will ask is this a multi threading issue if it is called before completion (of that is really happening).

Sorry if I'm really missing the boat here.

What is really happening is:

• This is an NT design bug, not a multi-threading issue
• The constructor calls percolate up to the NT constructor. No indicator constructor gets to do anything but pass the call along. In particular no indicator constructor code gets to run yet.
• The NT constructor decides the State is now SetDefaults, and calls the indicator's OnStateChange(). Note that none of the indicator's constructor code has yet run, so NT is making a method call on an object that has not yet been constructed. In the opinion of at least some of us that is Pure Evil -- a major bug in NT
• The NT constructor code returns. At that point the indicator constructor code finally gets to run.

My thoughts about this:

• It is purely Evil. No matter what code you think has what responsibility, making a call on an object that has not yet been constructed is a major bug. NT is doing so deliberately, so that makes it a major design bug.
• While it remains a bug in any case, the bug may not affect you if you do not have a constructor that does something that needs to be done before OnStateChange() runs. In my case I do have such a thing. Most indicators do not.
• There are easier and harder ways for NT to fix the problem, but Brett has indicated that NT is unlikely to fix their bug before release, so anyone who cares needs to figure out how to work around it.
• I have been back and forth on how to deal with the NT bug. One thing to note when you get to thinking about it is that the State is guaranteed to be State.SetDefaults by the time the first indicator constructor code gets to run (at least for the current implementation).

Indicator solutions

• You could ignore calls to OnStateChange() for SetDefaults and do what you would have done there in your constructor. The State will be correct and you will be operating in a conventional manner. Brett has agreed that he sees no problem with doing this. My only concern is the future -- are you exposing yourself to problems once they do get around to fixing this bug?
• What I finally decided to do is a little more complicated, but I think more robust in the future if NT changes what it does. Here it is, and it seems to be working fine for me.

1. OnStateChange() checks to see whether or not the constructor has completed. If construction has completed, it proceeds normally; if not, it just sets a flag and returns.
2. When the constructor has finished its work and is about to return it checks that flag. If the flag is set the constructor calls OnStateChange() itself, replacing the ignored call.
3. I feel that doing it this way is the way that is least likely to have future problems. It works today, and it will Just Work if NT fixes their bug.

FWIW: Mr. K and I have both observed that the call to OnStateChange() for SetDefaults is arguably a silly wasted call. The fact is that by the time any indicator constructor code gets to run the state is already SetDefaults, so the wasted call could just be omitted all together and developers told to do their SetDefaults work in the constructor.

--EV

[koganam]

Thanks for your reply. I'll give that a try. I do not see any downside.

I trust you noticed that Koganam now agrees with me
It really is a bad design that needs to get fixed. The fix I suggest is very simple, but would break code -- better now than after release. There are fixes that should not break code, such as waiting until after the constructor returns before calling OnStateChanged(), but they would probbly be more work for NT developers.

BTW: what's up with OnStateChanged() getting called twice before the constructor has returned, both times with the same state (SetDefaults)? Clearly the second time is not a state change. You should at least fix that, or you risk a problem with a developer who cannot tolerate the redundant (erroneous?) call.

--EV

That would only be if that really is what is happening. I find it a tad strange to even be able to run code in an object that has not yet been fully created. I just have a niggling feeling that there is something else happening that we are not aware of. For one thing, looking in the magic code, I can see that a cache is being populated with a copy of the object.

Maybe I will write some code to specifically profile what is happening to see if I can get to the root of it, albeit given that even when I supply an instance constructor, I really do nothing more than populate initial values in it, this would hardly impact me.

Still, it is strange to be seeming to run code in an object that technically does not yet exist. For now, I will have to suspend judgement on what is actually happening, but I still stand by the statement that it is bad, if true.

Just my $0.02.