ArgumentNullException in DynamicILGenerator.Emit

sharpjs commented 2 years ago

Describe the bug

While on call for an application that uses Insight.Database, I observed an ArgumentNullException thrown from DynamicILGenerator.Emit as invoked from Insight:

Value cannot be null.
Parameter name: meth

Here is the stack trace, with unrelated parts (my app, async cruft) removed:

System.ArgumentNullException:
   at System.Reflection.Emit.DynamicILGenerator.Emit (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at Insight.Database.CodeGenerator.ClassDeserializerGenerator.CreateGraphDeserializerWithCallback (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd (mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089)
   at Insight.Database.CodeGenerator.DbReaderDeserializer.GetDeserializer (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.CodeGenerator.DbReaderDeserializer.GetDeserializerWithCallback (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.OneToOne`1.GetRecordReader (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.DBConnectionExtensions+AsyncReader`1+<MoveNextAsync>d__11.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.DBConnectionExtensions+<ToListAsync>d__34`1.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.ChildRecordReader`3+<Insight-Database-Structure-IChildRecordReader<TResult\,TId>-ReadAsync>d__4.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.Children`3+<ReadAsync>d__4.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.QueryReader`1+<ReadChildrenAsync>d__8.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.ListReader`1+<ReadAsync>d__6.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.ResultsReader`3+<ReadAsync>d__10.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.Structure.ResultsReader`3+<ReadAsync>d__8.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   at Insight.Database.DBConnectionExtensions+<ExecuteAsyncAndAutoClose>d__36`1.MoveNext (Insight.Database.Core, Version=6.3.1.0, Culture=neutral, PublicKeyToken=f35c6ef87e515afc)
   ...unrelated stuff below here...

Steps to reproduce

I'm unsure how the application got there and not sure how much bandwidth I have for further research, but I can point to exactly where and why the exception was thrown from Insight's point of view. I have confirmed this by inspecting a debug snapshot taken at the time of the exception.

The exception is thrown on this line because deserializers[i] is null.

deserializers is produced by CreateDeserializersForSubObjects, which in turn uses CreateValueDeserializer and/or CreateClassDeserializerDynamicMethod.

CreateValueDeserializer cannot produce a null deserializer.

CreateClassDeserializerDynamicMethod can produce a null deserializer via this null return. In fact, that's the only possibility for a null deserializer. It must be the culprit.

Expected behavior

A few options come to mind:

This loop could be hardened to handle a null deserializer.
This null return could be changed to instead throw some helpful exception telling the programmer what they've done wrong.

No exception thrown, or some exception thrown that tells me what I've done wrong.

Dotnet version: net48
Database: Azure SQL Database
Library version: 6.3.1

jonwagner commented 2 years ago

Thanks for the diagnosis. 6.3.11 will have a fix for this (as much as I could reproduce)

sharpjs commented 2 years ago

The application in question has been updated to use a private build of Insight.Database that is v6.3.10 + your fix 9e871bf7bd2a89fdbf87b490647fdf0452fa3e63. I'll report back in a day or two with results.

So far, neither I nor a colleague have been able to reproduce this, but telemetry indicates that some users have been hitting it several times a day.

sharpjs commented 1 year ago

Yes, this fix fixed it for the application in question.

Jaxelr commented 1 year ago

Closing.

sharpjs commented 1 year ago

@jonwagner / @Jaxelr Looks like this fix didn't make it into 6.3.11. Any chance we could get 93968d7 into main?

jonwagner / Insight.Database