Collection was modified

[tradesmart]

It looks like following the OnRenderTargetChanged()

http://ninjatrader.com/support/helpGuides/nt8/en-us/?onrendertargetchanged.htm

recommendations has improved CPU and memory usage significantly.

Today, I'm running the version that uses a for loop (ScanMode 2 in my earlier post). It threw the "Collection was modified error." How is that possible? Must I revert to using .ToList()?

3. Before if (cdict_dictionaries.ContainsKey(i_barkey))
5. Before scanning cdict_dictionaries[i_barkey]
Collection was modified; enumeration operation may not execute.
Source: mscorlib
StackTrace: at System.Collections.Generic.Dictionary`2.Enumerator .MoveNext()
at System.Linq.Enumerable.ElementAt[TSource](IEnumerable`1 source, Int32 index)
at NinjaTrader.NinjaScript.Indicators.gws.gLiquidity. OnRender(ChartControl chartControl, ChartScale chartScale)
TargetSite: Boolean MoveNext()
ChartBars: (Base 1-Tick) SecondBarsType (Second: 6/4) TickReplay OnPriceChange

[NinjaTrader_Jesse]

Hello tradesmart,

I'm glad to hear that yielded some performance improvements, that's always a good step in the right direction.

Regarding the collection modified error, I would still have the same comment or that further debugging would be needed to fully understand why the exception is happening.

It seems that using .ToList has previously solved this problem for you, if that is the case I would likely suggest to use this until you have further debugged the script to find the real reason the error is occurring. This would allow you to continue developing the script at the least, this also wouldn't really be a "band-aid" so to speak but is another way to accomplish the same goal. If there is minimal performance impact from using.ToList I really can't see any reason to avoid it.

I look forward to being of further assistance.

[tradesmart]

Thanks, Jesse. I do like to understand all the ramifications of using one scanning technique versus another.

My best guess at this point is that OnMarketDepth(), firing asynchonously with OnRender() occasionally alters the dictionary being scanned. This only happens every now and then, anytime between hours and days. Using the for loop assumes that the cdict_dictionaries.Count remains constant throughout the loop doesn't it? This cannot be guaranteed, however, since OnMarketDepth() might have encountered an Operation.Add in the interim.

So, if my understanding of ToList() is correct that it takes a snapshot of the dictionary at a moment in time, iterating over the snapshot will never encounter a change from OnMarketDepth() until another snapshot is taken. The only drawback is that the dictionary is now being stored twice, but it's a small one so, as you say, a small price to pay.

[soulfx]

Hello, having not experienced this error for weeks with one of my custom indicators ... today i have the error 3 times - i wondered tradesmart if you have successfully resolved this and if so which of the iterating methods you outlined in post #10 you had success with ? It may be you changed your OnRender approach altogether. To reiterate the for loop i use of the dictionaries is NOT modifying anything it simply reads data to format output in OnRender. Any input appreciated so i can consider making the changes needed as with today having the error 3 times something is up even though i have not had the error for over 2 weeks at least. I am running 8.0.4 still so not sure if upgrading has resolved or fixed potential issues in this regard.
thanks

[tradesmart]

Hello, having not experienced this error for weeks with one of my custom indicators ... today i have the error 3 times - i wondered tradesmart if you have successfully resolved this and if so which of the iterating methods you outlined in post #10 you had success with ? It may be you changed your OnRender approach altogether. To reiterate the for loop i use of the dictionaries is NOT modifying anything it simply reads data to format output in OnRender. Any input appreciated so i can consider making the changes needed as with today having the error 3 times something is up even though i have not had the error for over 2 weeks at least. I am running 8.0.4 still so not sure if upgrading has resolved or fixed potential issues in this regard.
thanks

In all of my indicators that iterate through the Dictionary type in OnRender() I've been using the for loop (had only one occurrence of the error with that technique, never repeated). ToList is probably even less likely to throw the error, but the for loop feels more efficient to me. In the code for my indicator that was the main culprit, I went a step further and changed from using Dictionary.Add to ConcurrentDictionary.TryAdd in OnMarketData() with no errors thrown thereafter. https://www.dotnetperls.com/concurrentdictionary

[soulfx]

So an update on this since running for about 4 days after having made the changes in OnRender to use the for loop method to process the dictionaries (as outlined in #10) : With several days of reasonable volatility the charts seem more stable with this change and the display of data via OnRender. And so far have not had an incident of the 'collection modified' error - i have this custom indicator running on several charts and has not arisen on any of those. So as per your situation i was NOT trying to modify the collection i was iterating over which was Jesse's point earlier in the thread - so it seems this is required regardless of whether modifying or just reading and displaying the values in SharpDX draw methods in OnRender. Thanks tradesmart for your testing of this prior which enabled me to confidently make the changes needed.

[tradesmart]

Glad to hear that the for loop is working well for you.

For new indicators, taking a tip from SuperDomColumns @Volume.cs, I'm going to use ConcurrentDictionary. As I get around to it, I plan to convert any indicators that still use System.Collections.Generic.Dictionary to System.Collections.Concurrent.ConcurrentDictionary and populate it via .TryAdd() or .AddOrUpdate().

For example, in OnMarketData():

uint ui_volume ;
lock (e.Instrument.SyncMarketData)
{
        ui_volume = (uint)e.Volume ;
        _cdictionary.AddOrUpdate(e.Price, ui_volume, (price, oldVolume) => oldVolume + ui_volume);
}

[soulfx]

Hello, even better some new work with dictionaries this reared its head again - So now as per your note i am using ConcurrentDictionary - so far seems to work well and is necessary when processing alot of update events. Will review other work and likely migrate those over too.
thanks again

[satoyama]

convert to for loop

hello,

In an attempt to avoid the "Collection was modified; enumeration operation may not execute." error, how do I convert
foreach (DrawingTool dt in DrawObjects)
to a "for loop"?

I know it is like a beginners question, but thanks in advance anyway to anyone who can help.

[NinjaTrader_Jesse]

Hello satoyama,

Thank you for the inquiry.

You would likely need to do as the prior users have done and use .ToList(). accessing the collection directly can lead to the error you posted.

List<IDrawingTool> objs = DrawObjects.ToList();
for(int i = 0; i < objs.Count; i++)
{
	IDrawingTool obj = objs[i];
	Print(obj.GetType().Name);
}

If you wanted specifically a for loop, you could do it like the above sample.

I look forward to being of further assistance.

[td_910]

Hi,
I get the same error, but do not use ANY foreach in the code. Is there anything else, that could throw that error?
Like the original poster mentions: "Unfortunately, this is one of those intermittent errors that is not readily reproducible. "

Another question: Do I need to check, if a draw object exists before removing it?

[NinjaTrader_ChrisL]

Hello td_910,

Thanks for your note.

Are there any arrays or lists being used in your script? I would recommend using Visual Studio so you can break on exceptions to see what is wrong. Here is a short video on how to set this up:

https://clauber.tinytake.com/sf/MzQyMzcxOF8xMDI0MzIyMw

I look forward to assisting further.

[td_910]

No, I haven't defined any arrays or lists.
Does it matter, that I'm trying to remove objects, that don't exist?

Thanks, I'm no programmer, just a trader, but I try to look into that.

[NinjaTrader_ChrisL]

Hello td_910,

Thanks for your reply.

You will get an indexing error if you try to access a drawing object that does not exist. Could you export and post your code so I can see how the draw objects are being accessed?

I look forward to your reply.

[td_910]

Hi Chris,

this is drawing a false breakout of the 1st bar of the day. the code runs Calc.onbarclose

https://www.screencast.com/t/fcdc3KXcq5cc

            #region B1
            bool Bar1LongxtF = false;
            if (showHTF_B1 && Bars.BarsSinceNewTradingDay>0 &&
                !(High[Bars.BarsSinceNewTradingDay]<High[Bars.BarsSinceNewTradingDay-1] && Low[Bars.BarsSinceNewTradingDay]>Low[Bars.BarsSinceNewTradingDay-1] && Bars.BarsSinceNewTradingDay>2) &&
                MAX(High, Bars.BarsSinceNewTradingDay)[1]<High[0] && Math.Abs(curhigh-High[Bars.BarsSinceNewTradingDay]-x1TF)<Math.Max(band,0.2*TickSize))
            {
                Draw.Text(this, "Long_B1_TickFailure"+unique+(CurrentBar), true, (x1TF/TickSize).ToString("N0")+"tF\nb1", 0, High[0], (int)(1.5*toffset), bear_Symbol_Color, myFont, TextAlignment.Center, Brushes.Transparent, ChartControl.Properties.ChartBackground, 8);
                Bar1LongxtF = true;
            }

            //Remove untriggered
            if (High[0] > High[1])
                if (!Test)
                {
                    RemoveDrawObject("Long_B1_TickFailure"+unique+(CurrentBar - 1));
                    RemoveDrawObject("1Long1tF_Line"+unique+(CurrentBar - 1));
                }
                else
                //grey sinals
                {
                    if (DrawObjects["Long_B1_TickFailure"+unique+(CurrentBar-1)] != null)
                    {
                        dynamic LongText = DrawObjects["Long_B1_TickFailure"+unique+(CurrentBar-1)];
                        if (LongText.ToString().Equals("NinjaTrader.NinjaScript.DrawingTools.Text"))
                            LongText.TextBrush = old_Symbol_Color;
                    }
                }
            #endregion

the user can select (Test) , whether to remove or grey "old" signals

the RemoveDrawObject runs regardless whether the object exists or not
I only check if the object exists, if I change its text color