4

Closed

IntegerExtension.ToStringLookup not threadsafe

description

Hi,
We are using this library in our service which generates several reports in parallel threads and we're getting the following exceptions:

System.ArgumentException: An item with the same key has already been added.
at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
at ClosedXML.Excel.IntegerExtensions.ToStringLookup(Int32 value)

This error is caused by those lines of code (of static class ClosedXML.Excel.IntegerExtensions):
private static readonly Dictionary<int, string> intToString = new Dictionary<int, string>();
public static string ToStringLookup(this int value)
{
  if (!IntegerExtensions.intToString.ContainsKey(value))
    IntegerExtensions.intToString.Add(value, value.ToString((IFormatProvider) IntegerExtensions.nfi));
  return IntegerExtensions.intToString[value];
}
I think replace this dictionary by ConcurrentDictionary and use AddOrUpdate method will do the trick :)

Regards,
Maciej.

file attachments

Closed Aug 7 at 5:40 AM by NickNack2020
Merged into master

comments

SpyderTech02 wrote Nov 8, 2013 at 3:43 PM

We're seeing the same exception. In case it helps, here's a couple example stack traces, but there are several different stack traces that we've hit this exception with.

System.IndexOutOfRangeException: Index was outside the bounds of the array. at System.Collections.Generic.Dictionary2.Insert(TKey key, TValue value, Boolean add) at ClosedXML.Excel.IntegerExtensions.ToStringLookup(Int32 value) at ClosedXML.Excel.XLAddress.ToString() at System.String.Concat(Object arg0, Object arg1, Object arg2) at ClosedXML.Excel.XLRangeAddress.ToString() at ClosedXML.Excel.XLRangeBase.Merge(Boolean checkIntersect) at ClosedXML.Excel.XLRangeBase.Merge()

System.IndexOutOfRangeException: Index was outside the bounds of the array. at System.Collections.Generic.Dictionary
2.Insert(TKey key, TValue value, Boolean add) at ClosedXML.Excel.IntegerExtensions.ToStringLookup(Int32 value) at ClosedXML.Excel.XLWorkbook.GenerateWorksheetPartContent(WorksheetPart worksheetPart, XLWorksheet xlWorksheet, SaveContext context) at ClosedXML.Excel.XLWorkbook.CreateParts(SpreadsheetDocument document) at ClosedXML.Excel.XLWorkbook.CreatePackage(String filePath) at ClosedXML.Excel.XLWorkbook.SaveAs(String file)

System.Collections.Generic.KeyNotFoundException: The given key was not present in the dictionary. at System.Collections.Generic.Dictionary`2.get_Item(TKey key) at ClosedXML.Excel.IntegerExtensions.ToStringLookup(Int32 value) at ClosedXML.Excel.XLAddress.GetTrimmedAddress() at ClosedXML.Excel.XLWorkbook.GenerateWorksheetPartContent(WorksheetPart worksheetPart, XLWorksheet xlWorksheet, SaveContext context) at ClosedXML.Excel.XLWorkbook.CreateParts(SpreadsheetDocument document) at ClosedXML.Excel.XLWorkbook.CreatePackage(String filePath) at ClosedXML.Excel.XLWorkbook.SaveAs(String file)

SpyderTech02 wrote Nov 8, 2013 at 4:57 PM

Something like this should work...

public static class IntegerExtensions { private static readonly NumberFormatInfo nfi = CultureInfo.InvariantCulture.NumberFormat; private static readonly ConcurrentDictionary intToString = new ConcurrentDictionary(); public static String ToStringLookup(this Int32 value) { return intToString.GetOrAdd(value, v => v.ToString(nfi)); } }

NickNack2020 wrote Jul 31 at 3:42 AM

I did some quick testing, I am not even sure the caching in this case is getting us much of a benefit.

I ran multiple test, and it always seems faster to just to string the int on my machine, then it does to do the dictionary lookup.

Not to mention, if there are many numbers, I could watch the memory grow in task manager. It may help our memory consumption to ditch this cache. Especially for large workbooks.

What are your thoughts. Would you like me to proceed with this change?

MDeLeon wrote Jul 31 at 4:01 AM

Do it.

NickNack2020 wrote Jul 31 at 5:15 AM

Alright. I will work on this. My hope is to slowly tackle these thread safe issues.