search

NotAdam / Lumina

A simple, performant and extensible framework for interacting with FFXIV game data
Do What The F*ck You Want To Public License
86 stars 54 forks source link

`EmptyLazyRow.GetFirstLazyRowOrEmpty` Concurrency Issue #85

Open WorkingRobot opened 1 week ago

WorkingRobot commented 1 week ago

https://github.com/NotAdam/Lumina/blob/bb7d83631a3e878017f16ee84e42e9f794868b88/src/Lumina/Excel/LazyRow.cs#L32

This being a static non-concurrent dictionary can cause concurrency issues when parsing new rows. Here is a stacktrace I've seen regarding this issue:

System.Collections.Generic.KeyNotFoundException: The given key 'ChocoboTaxiStand' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Lumina.Excel.EmptyLazyRow.GetFirstLazyRowOrEmpty(GameData gameData, UInt32 row, Language language, String[] sheetNames)
   at ExdSheets.Action.PopulateData(RowParser parser, GameData gameData, Language language)
   at Lumina.Excel.ExcelSheet`1.ReadRowObj(RowParser parser, UInt32 rowId)

Obviously in code like the following, a dictionary lookup like that (L63) should never fail unless there is a race condition of some sort. https://github.com/NotAdam/Lumina/blob/bb7d83631a3e878017f16ee84e42e9f794868b88/src/Lumina/Excel/LazyRow.cs#L53-L66

NotAdam commented 1 week ago

ah yeah that was likely an oversight when initially changing this to work a bit nicer in async/threaded environments - happy for this to move to a concurrentdict though if you'd like to PR it, as there's not really a trivial lock that would make this work nicely i think