New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread safety of context item collections #1043

Closed
chrisbigart opened this Issue Jan 24, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@chrisbigart

chrisbigart commented Jan 24, 2018

We have code that calls .First() on a collection in a Microsoft.Dynamics.CRM.System context that is deadlocked because of the lack of thread-safety of the Dictionary<> class. A process dump shows that multiple threads are in the EntityTracker.InternalAttachEntityDescriptor() method, calling the .Add()/.Insert() method of the 'identityToDescriptor' Dictionary<>.

The calling code looks like:

public static string GetContactById(Microsoft.Dynamics.CRM.System _context, Guid id)
        {
            var contact = _context.Contacts.Expand(a => a.Parentcustomerid_account).Where(a => a.Contactid == id);
            var cContact = ConvertContact(_context, contact.First());
            return Util.Serialize<Business.Contact>(cContact);
        }

It appears that the fix would be to change each Dictionary<> to a ConcurrentDictionary<> for the EntityTracker internal members: 'entityDescriptors', 'identityToDescriptor', and 'bindings'.

If there is some other work-around, please let me know. Thanks.

@chrisbigart

This comment has been minimized.

chrisbigart commented Jan 24, 2018

The call stack looks like:

 >mscorlib.dll!System.Collections.Generic.Dictionary<object, Microsoft.OData.Client.EntityDescriptor>.Insert(object key, Microsoft.OData.Client.EntityDescriptor value, bool add)
 
 >Microsoft.OData.Client.dll!Microsoft.OData.Client.EntityTracker.InternalAttachEntityDescriptor(Microsoft.OData.Client.EntityDescriptor entityDescriptorFromMaterializer, bool failIfDuplicated) Line 286	C#	

 >Microsoft.OData.Client.dll!Microsoft.OData.Client.AtomMaterializerLog.ApplyToContext() Line 68	C#	

 >Microsoft.OData.Client.dll!Microsoft.OData.Client.MaterializeAtom.MoveNextInternal() Line 280	C#	

 >Microsoft.OData.Client.dll!Microsoft.OData.Client.MaterializeAtom.MoveNext() Line 217	C#	

 >System.Core.dll!System.Linq.Enumerable.<CastIterator>d__b1<2016WebApiService.Microsoft.Dynamics.CRM.Contact>.MoveNext()
 
 >System.Core.dll!System.Linq.Enumerable.First<2016WebApiService.Microsoft.Dynamics.CRM.Contact>(System.Collections.Generic.IEnumerable<2016WebApiService.Microsoft.Dynamics.CRM.Contact> source)	

 >Microsoft.OData.Client.dll!Microsoft.OData.Client.DataServiceQueryProvider.ReturnSingleton<2016WebApiService.Microsoft.Dynamics.CRM.Contact>(System.Linq.Expressions.Expression expression) Line 67	C#

 >System.Core.dll!System.Linq.Queryable.First<2016WebApiService.Microsoft.Dynamics.CRM.Contact>(System.Linq.IQueryable<2016WebApiService.Microsoft.Dynamics.CRM.Contact> source)	

 >>2016WebApiService.dll!2016WebApiService.Business.Contact.GetContactById(2016WebApiService.Microsoft.Dynamics.CRM.System _context = {2016WebApiService.Microsoft.Dynamics.CRM.System}, System.Guid id = {System.Guid}) Line 67	C#

The version of the Microsoft.OData.* assemblies that are being used is 6.13.0.0, however, looking at the current code in the git repository, it appears that ultimately the same functions will be called, resulting in an .Add()/.Insert() on a thread-unsafe Dictionary<>.

@AlanWong-MS AlanWong-MS added the P4 label Jan 25, 2018

@AlanWong-MS

This comment has been minimized.

Contributor

AlanWong-MS commented Jan 25, 2018

@chrisbigart, thanks for filing the issue. Can you please open a pull request to this one with your proposed fix?

#905 and #990 serve as two references that you can follow. If we use ConcurrentDictionary, we have to make sure that we have the #if PORTABLELIB && WINDOWSPHONE as those platforms don't support ConcurrentDictionary.

@robertmclaws

This comment has been minimized.

Collaborator

robertmclaws commented Jan 25, 2018

So, I watched the other versions of these issues, and here are my concerns with ConcurrentDictionary:

  1. Why are we still compiling for WINDOWSPHONE when it is no longer supported?
  2. Why aren't we just using the portable version of System.Threading.Tasks that has ConcurrentDictionary in it?
@AlanWong-MS

This comment has been minimized.

Contributor

AlanWong-MS commented Jan 29, 2018

@robertmclaws, good questions.

  1. Why are we still compiling for WINDOWSPHONE when it is no longer supported?

We also asked that question to ourselves and we decided that as long as we still have tests running against it (to re-cap: our most recent statement was indeed that we're not supporting Windows Phone 8.0), we would still like to have our all of the tests buildable and passing. It would make sense for us to remove those WinPhone8.0 tests as well, but we currently don't have resources allocated to going through and cleaning up all WinPhone related items at the moment. Having said all that, it's less costly and risky at the moment to add another WINDOWSPHONE preprocessor.

  1. Why aren't we just using the portable version of System.Threading.Tasks that has ConcurrentDictionary in it?

I'm not sure what decisions were made regarding the choice of the ConcurrentDictionary version or why we have a non-concurrent version for the portable. I'll ask around, but we should test the portable version of ConcurrentDictionary since it exists.

@biaol-odata

This comment has been minimized.

Member

biaol-odata commented Jun 26, 2018

Close as fixed.

@biaol-odata biaol-odata added the fixed label Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment