NT8 sometimes runs OnStateChange termination during OnBarUpdate

[QuantKey_Bruce]

To spell it out further, it is easy to imagine that there is a worker pool of threads with, say, eight or sixteen threads in it. The programmer has an event to handle a depth event that takes 10ms to run on a slow(er) computer, at least on occasion. A fast market exists with more than 100 ticks in one second. While the depth event OnMarketDepth is still running, a second tick needs to be handled - if it is not, the platform will get backed up. If it waits for your event to exit, we could end up with a platform lock, and if it does not wait, it relies on you to handle reentrancy yourself.

However, if you handle it by the obvious choice of mutex locking, we're back to the same scenario - the first thread is still processing your event, and the next seven threads also end up waiting for the same lock, resulting in the entire pool being frozen. Depending on how the thread pool is configured, it might or might not fire up more threads if all of the threads are waiting, but even if it does, all of the subsequent threads it fires up will near-immediately wind up holding for the same lock.

That is why I asked NT how they expected, as a best practice, for contention to be handled, and whether we should do the locking ourselves, or how they recommended that it be done. It seems to me this that for those cases where the ticks (or depth events, or whatever) must be handled (fully) in order, locks may be a requirement so it puzzles me that we may be not recommended to go that route. The only alternative I can quickly foresee is that they handle it on the platform side (by only sending the events one at a time to a given indicator, waiting until the first one exits before sending the next one) but that is not the way it is working so far, and it adds complexity for them.

[ren37]

Bruce,

Can you explain to me why they lock this code?
I don't see operations with e.g. e.Instrument.MarketDepth.Asks inside it.
CLR just waits prior OnMarketDepth() call finished?
Full source http://ninjatrader.com/support/forum...ead.php?t=3478

       protected override void OnMarketDepth(MarketDepthEventArgs e)
        {
            // protect e.Instrument.MarketDepth.Asks and e.Instrument.MarketDepth.Bids against in-flight changes
            lock (e.Instrument.SyncMarketDepth)
            {
                List<LadderRow> rows = (e.MarketDataType == MarketDataType.Ask ? askRows: bidRows);
                
                if (e.Operation == Operation.Add)
                    rows.Insert(e.Position, new LadderRow { MarketMaker = e.MarketMaker, Price = e.Price, Volume = e.Volume });
                else if (e.Operation == Operation.Remove)
                    rows.RemoveAt(e.Position);
                else if (e.Operation == Operation.Update)
                {
                    rows[e.Position].MarketMaker    = e.MarketMaker;
                    rows[e.Position].Price            = e.Price;
                    rows[e.Position].Volume            = e.Volume;
                }
            }
        } 

[QuantKey_Bruce]

Bruce,

Can you explain to me why they lock this code?
I don't see operations with e.g. e.Instrument.MarketDepth.Asks inside it.
CLR just waits prior OnMarketDepth() call finished?
Full source http://ninjatrader.com/support/forum...ead.php?t=3478

Sure.

Regarding your question about there not being operations with asks in them, look at the usage of the ? operator there in setting which rows to operate on. The ? operator selects the ask rows for asks and bid rows for bids in this case, which is why after selecting the right rows, the code can proceed with the same flow of control from there on.

You will notice there is no corresponding lock to the NT8 example required in the NT7 example - that is because in NT7 they were sequentially executed, while in NT8 these method calls are the jobs in a pool of worker threads. The lock keyword takes a single argument (an object) and provides a mechanism to assure that the code that follows in the braced code block will never execute at the same time as itself or any other code block that locks the same object.

If they did not do this (in NT8) you could have two different depth events coming into the method in parallel, resulting in corruption of the ladder the method is keeping track of as they each step on the other trying to modify the same list.

This is exactly the sort of thing I was referring to.

[ren37]

I agrre about List<LadderRow> rows = (e.MarketDataType == MarketDataType.Ask ? askRows: bidRows); because askRows, bidRows are indicator class instance data shared among threads.

But e variable as I suppose is local data (resides in method args stack of current thread).
So e.MarketDataType == MarketDataType.Ask ? does not required lock() if they do like this OnMarketDepth(new MarketDepthEventArgs(...));
BUT if they do call like this OnMarketDepth(event) where event is a class member then lock() is required.

rows.Insert() and rows.RemoveAt() require lock() any way.

// protect e.Instrument.MarketDepth.Asks and e.Instrument.MarketDepth.Bids against in-flight changes
This explanation just puzzles because e.Instrument.MarketDepth.Asks/Bid refreshes inside NT core but not in user code bracketed by lock() operator

I think this code is equal. Have used this before.
Will try it with .ToDictionary() for custom order book dictionaries rendering in OnRender().
But not sure .ToDictionary() is multi-thread safe method. What do you think about it?

...
        object lockObj = new object();
...

        protected override void OnMarketDepth(MarketDepthEventArgs e)
        {
            lock (lockObj)
            {
                OrderBook.OnMarketDepth(e);  // custom order book logic
            }
        }


        // class OrderBook
        public void Draw(ChartControl chartControl, ChartScale chartScale)
        {
            // Clone rows dictionaries
            Dictionary<double, long> askRows = price2volume[(int)TradeSide.Ask].ToDictionary(kv => kv.Key, kv => kv.Value);
            Dictionary<double, long> bidRows = price2volume[(int)TradeSide.Bid].ToDictionary(kv => kv.Key, kv => kv.Value);
                 ...
         } 

[QuantKey_Bruce]

The comment could be better worded. Remember, each method invocation has its own reference to e, so that is not really the main issue here. What the lock is really protecting are your field variables askRows and bidRows. If the two simultaneous instances of OnMarketDepth each have their own "e" but are trying to simultaneously modify the same list, the list will become corrupted because lists are not inherently thread-safe. That is why they are using the lock - not to protect "e" itself which is probably correct in both cases.

Regarding .ToDictionary being thread-safe, it is not, but that is sort of missing the point because each instance of the OnMarketData method makes its own dictionary as a result of the call, so they can't see each other. What they do see of each other is they are accessing the same field variable bidRows and askRows or your price2volume, and those non-thread-safe lists must be protected by a lock during access if you don't want them to become corrupted by simultaneous attempts to change their contents, or by attempts to read their contents while the other thread is changing them.

[ren37]

Can't agree with you in some details...
Here is my template for thread safe Level 2 processing.
OnMarketDepth() and OnMarketData() are equal inside to OnRender()

        Mutex mutexObj = new Mutex(); 
...

        protected override void OnStateChange()
        {
            mutexObj.WaitOne();
            try
            {
...
                else if (State == State.Terminated)
                {
...
                    mutexObj.Dispose();
                    mutexObj = null;
                }
            }
            finally
            {
                if (mutexObj != null) mutexObj.ReleaseMutex();
            }
        }


        protected override void OnRender(ChartControl chartControl, ChartScale chartScale)
        {
            try
            {
                // mutexObj might be null or disposed in OnStateChanged()
                mutexObj.WaitOne();
            }
            catch (AbandonedMutexException)
            {
                // do nothing
#if DEBUG
                Print(GetHashCode() + " OnRender - AbandonedMutexException");
#endif
            }
            catch (Exception)
            {
                return;
            }

            try
            {
                // render data
            }
            finally
            {
                mutexObj.ReleaseMutex();
            }
        } 

[QuantKey_Bruce]

A mutex unlike a lock works across process boundaries (though all NinjaTrader threads are in the same process), but a mutex takes around fifty times as long to acquire and release. What leads you to use a mutex in this case?

[ren37]

A mutex unlike a lock works across process boundaries (though all NinjaTrader threads are in the same process), but a mutex takes around fifty times as long to acquire and release. What leads you to use a mutex in this case?

lock() is useless in data concurrent acess management because in this case different code on common data executed in concurrent threads. "A mutex unlike a lock works across process boundaries (though all NinjaTrader threads are in the same process)," So this is the cause.

1. Actually first we need a queue to process e.g. market depth data. Commands in data feed for Level 2 definitely been serialized because they are market depth position based. Mutex just "queued" incoming data. Perhaps we could use thread-safe Queue.Enqueu() to accumulate commands and Enqueu.Dequeu() to retreive data for processing. Not sure.

2. Mutex solves issue with OnStateChange() - State == State.Terminated call during OnRender(), OnBarUpdate() etc. execution in concurrent thread.

Now I also use lock() inside OrderBook.OnMarketData(e) (it's redundant now but remains for NT7 backward compatability of my base library, will see) and .ToDictionary() in OnRender().

[QuantKey_Bruce]

lock() is useless in data concurrent acess management because in this case different code on common data executed in concurrent threads.

A lock works based on the object it locks (the object in parenthesis after the keyword) not based on the code block encompassed by the following braces, so two code blocks that lock the same object are mutually excluded for concurrent execution. Different threads within the same process e.g. all within NinjaTrader.exe are still protected by a lock on the same object within any one of them because they all share the same memory space.

So, in other words, you do not have to go from a lock to a mutex just because you have two code blocks that need to concurrently preclude each other - they just have to lock the same object. Does that make sense?

[tradesmart]

How about ConcurrentDictionary? https://msdn.microsoft.com/en-us/lib...v=vs.110).aspx
I see that several of the SuperDomColumns use it.

[ren37]

Ok, agree

But I don't like this

        protected override void OnBarUpdate()
        {
            try
            {
                // lockObj might be null or disposed in OnStateChanged()
                lock(lockObj)
                {
                    // algo
                }
            }
            catch (Exception)
            {
                // silent all exceptions from lockObj and algo
            }
        } 

On the other hand lockObj no need to be disposed from indicator code.
So lock() is ok.

[QuantKey_Bruce]

How about ConcurrentDictionary? https://msdn.microsoft.com/en-us/lib...v=vs.110).aspx
I see that several of the SuperDomColumns use it.

Sure, that is not an unreasonable approach to use a thread-safe library class instead of a non-thread-safe one, but remember the ladder example is just an example provided by another user ren37. It's a good example and is helpful to the discussion, but concurrency issues can be a lot of different things besides contention for a collection. The actual issue reported in this thread is that Terminated state changes happen while OnBarUpdate is running, which is much more pervasive, and means just about anything might be cleaned up while the other method is running its code. That is how this discussion led to lock and mutex as ways to prevent that from happening, and a ConcurrentDictionary may make the one collection thread-safe but does not address the real problem here.

[koganam]

In the beta phase, I had posted a thread on this issue at http://ninjatrader.com/support/forum...ad.php?t=83542 with some context. It seems that NT8 NinjaScript runs OnStateChange to change the state to State.Terminated concurrently with execution of OnBarUpdate.

I have updated my demonstration indicator to use a Print statement rather than throwing an exception, because it now seems that throwing an exception during the StateChange to State.Terminated can hang the chart indefinitely.

Here's the demonstration code:

public class TestPrematureTermination : Indicator
{
private bool IsUpdating = false;

protected override void OnStateChange()
{
if (State == State.SetDefaults)
{
Description = @"Test premature termination issue";
Name = "TestPrematureTermination";
}
else if (State == State.Terminated)
{
if (IsUpdating) Print("Broken");
}
}

protected override void OnBarUpdate()
{
IsUpdating = true;
System.Threading.Thread.Sleep(1);
IsUpdating = false;
}
}

If you simply place this on a chart - for instance, 30 days of 1 minute bars of ES - and hit F5 a few times, you will see the message Broken in the output window, indicating that the unmanaged resources are being destroyed during OnBarUpdate's execution.

Because the stated intention is for us to clean up unmanaged resources during OnStateChange to State.Terminated, this results in exceptions during the execution of OnBarUpdate which relies on those same resources. Shouldn't NT8 wait for OnBarUpdate to exit before changing the state to State.Terminated?

The alternative seems to be that we introduce our own object locking, but this has its own issues.

Please confirm that this is what you expect us to do, in order to avoid exceptions happening that can lock the chart.

In the literature at the link that I posted before: http://ninjatrader.com/support/helpG...fecycle_of.htm, it is stated that because of the somewhat involved way that object states are handled, it is necessary to ensure that the State.Terminated trigger does not originate from outside the object extant, before running the State.Terminated code block. Are you saying that that approach is still insufficient?

That would make your example look like this:

[COLOR="Blue"][B]private bool markerForThisIndi = false;[/B][/COLOR]

public class TestPrematureTermination : Indicator
{
private bool IsUpdating = false;

protected override void OnStateChange()
{
if (State == State.SetDefaults)
{
Description	= @"Test premature termination issue";
Name	= "TestPrematureTermination";
}
  else if (State == State.Historical)
  {
    // when indicator starts historical processing

   [COLOR="Blue"] [B]markerForThisIndi = true;[/B][/COLOR] // use a flag to track this logic was executed
  }
else if (State == State.Terminated)
{
if (IsUpdating && [COLOR="blue"][B]markerForThisIndi[/B][/COLOR]) Print("Broken");
}
}

protected override void OnBarUpdate()
{
IsUpdating = true;
System.Threading.Thread.Sleep(1); 
IsUpdating = false;
}
}

[QuantKey_Bruce]

In the literature at the link that I posted before: http://ninjatrader.com/support/helpG...fecycle_of.htm, it is stated that because of the somewhat involved way that object states are handled, it is necessary to ensure that the State.Terminated trigger does not originate from outside the object extant, before running the State.Terminated code block. Are you saying that that approach is still insufficient?

Well, all that flag accomplishes is to make sure that we don't, for instance, dispose of an object before it's been set up. If you think about it, if you do something like if (a != null) a.dispose(); you should not need a flag for whether or not a has been allocated, provided you allocate it in the Configure or later state. Could you clarify what the issue would be with that deallocating only if it is allocated? If NT bungles the object state order and calls Terminated before calling Configure (or where ever the allocation happens) it would just not be allocated yet and so would be unavailable to deallocate...

Reading over the NT documentation example of adding and later removing a toolbar, they are simply trying to show how to handle it if you had something that wasn't so simple e.g. you couldn't quickly check if (a != null) and needed to know if the toolbar was added before you try to remove it.

[koganam]

Well, all that flag accomplishes is to make sure that we don't, for instance, dispose of an object before it's been set up. If you think about it, if you do something like if (a != null) a.dispose(); you should not need a flag for whether or not a has been allocated, provided you allocate it in the Configure or later state. Could you clarify what the issue would be with that deallocating only if it is allocated? If NT bungles the object state order and calls Terminated before calling Configure (or where ever the allocation happens) it would just not be allocated yet and so would be unavailable to deallocate...

Reading over the NT documentation example of adding and later removing a toolbar, they are simply trying to show how to handle it if you had something that wasn't so simple e.g. you couldn't quickly check if (a != null) and needed to know if the toolbar was added before you try to remove it.

Agreed with the larger purpose of your statement here, but actually the problem was discovered a while back in a very trivial situation.That whole discussion is in the thread to which I linked in my first response.

I was just kind of wondering if the case/example that you describe would not be susceptible to being handled as a similar rather simple case. I have no doubt that we are going to hit more complex scenarios, where a lock or mutex will pretty much be the realistic solution.

It is just that so far I have not come against any such case, as the flag has pretty much been reliable enough to prevent premature disposal where it used to be a real situation breaker.