3

Closed

Multithreading- AdjustToContents not threadsafe

description

Hi,

I have recently upgraded from 0.65 to 0.73 and while I have seen some performance improvements, several reports that run in parallel threads crashed in my UAT environment. It seems the problem is in the AdjustToContents() method.

The full exception is:

---> System.InvalidOperationException: Object is currently in use elsewhere.
at System.Drawing.Graphics.MeasureString(String text, Font font, SizeF layoutArea, StringFormat stringFormat)
at System.Drawing.Graphics.MeasureString(String text, Font font, Int32 width, StringFormat format)
at ClosedXML.Excel.FontBaseExtensions.GetWidth(IXLFontBase fontBase, String text, Dictionary2 fontCache)
at ClosedXML.Excel.XLColumn.AdjustToContents(Int32 startRow, Int32 endRow, Double minWidth, Double maxWidth)
at ClosedXML.Excel.XLColumns.<AdjustToContents>b__8(XLColumn c)
at System.Collections.Generic.List
1.ForEach(Action`1 action)
at ClosedXML.Excel.XLColumns.AdjustToContents()
Closed Aug 7 at 4:38 AM by NickNack2020
Merged into master.

comments

metalrose wrote Jul 29 at 9:13 AM

Are you filling in reports whilst simultaneously adjusting to contents ? Can you not just call Adjust To Contents when your reports are all filled in ?

Tornhoof wrote Jul 29 at 10:08 PM

The error is simple:
In at ClosedXML.Excel.FontBaseExtensions.GetWidth(IXLFontBase fontBase, String text, Dictionary`2 fontCache)
the library is using a static graphics object. Graphics objects are not threadsafe.
You need to create a new one each time you use it or lock it appropriately.

The workaround for this problem is locking the Code which calls AdjustToContents.

MDeLeon wrote Jul 29 at 10:33 PM

Thanks Tomhoof. I'm leaning towards locking it in GetWidth before using it. What do you think?

NickNack2020 wrote Jul 30 at 12:23 AM

If this is not a time consuming operation, that I agree with locking.

Tornhoof wrote Jul 30 at 6:15 AM

Not sure if locking is the right way here, the methods GetWidth and GetHeight are called many times throughout the AdjustToContents function. Locking is not a fast process.
I would recommend passing a graphics object down from the AdjustToContents method to GetWidth and GetHeight and get rid of the static object. This will be thread-safe and get rid of the locking.

While you are at it, two further thread-safety issues in the Extensions class.

1.IntegerExtensions.ToStringLookup:
  • A normal Dictionary is not threadsafe, especially not writing to it, either lock it or use ConcurrentDictionary. I'm not sure about the dictionary there at all, it might get really huge very fast, considering that you try to cache each and every integer you pass into it. I'm not sure about the performance gain by this lookup. If it is really that much faster, cache the lookup locally in the method you need it in.
  • StringExtensions.FixNewLines
  • Regex object is not threadsafe, simply use the static Regex.Replace/Match etc. methods and .NET will automatically cache the Regex objects for it and make sure it's thread-safe. In that case you should not use the RegexOptions.Compiled option.

Tornhoof wrote Jul 30 at 7:13 AM

For the sake of completeness, I did a short look through the source code of the 0.73 release and found the following issues wrt thread-safety, mostly the above repeats throughout the code.

XLCFConverters: Dictionary, only read
MathTrig.cs: Random
XLCell.cs: Regex, Dictionary, only read
XLColor.cs: Dictionaries, both r/w
Extensions.cs: Regex, Dictionary (r/w), Graphics object
PathHelper.cs: Regex
XLHelper.cs: Regex, Dictionaries (r/w)
ResourceFileExtractor.cs: Dictionary with cached objects, the lock is wrong here: lock (ms_defaultExtractors) (don't lock the object, use a lock object)

I personally think that most of the thread issues are not as important as the graphics object issue. The impact should be smaller.

InfiniteLoop12 wrote Jul 30 at 8:12 AM

Re: the IntegerExtensions.ToStringLookup threading issues, I think someone has raised a workitem about it before.

Threading issues with dictionaries is the main reason why this library is not 100% threadsafe, it would be great if they were sorted.

NickNack2020 wrote Aug 1 at 1:52 AM

NickNack2020 wrote Aug 1 at 3:33 AM

NickNack2020 wrote Aug 1 at 3:34 AM

Issue for IntergerExtensions.ToStringLookup https://closedxml.codeplex.com/workitem/9118

NickNack2020 wrote Aug 1 at 3:53 AM

Alright, that is it for me tonight.

Here is the pull request to make AdjustToContents thread safe

https://closedxml.codeplex.com/SourceControl/network/forks/NickNack2020/ClosedXml/contribution/7227

I will continue to look into Tornhoof's list when I get a chance if no one else does.

Thank you very much for the list.