NullReferenceException with async code
developermj opened this issue · 13 comments
I am using this project to save files to a file system in a transaction with an ef core database transaction. My code is leveraging multiple async methods within services along with ef core async methods. The code fails upon calling of the transaction scope objects Complete method. After pulling the code and reviewing I found that removing the [ThreadStatic] attribute in the TxFileManager class on the _enlistments dictionary to resolve the issue for me.
After thinking through this I'm not sure why the ThreadStatic attribute would be required. I would think that because transactions can run across multiple threads that we would want to keep track of the enlistments the same way.
I can create a PR for this work if anybody finds this valuable. TIA.
Hi: The use of ThreadStatic was pretty important if I remembered correctly. I wrote about it in my initial blog post.
Would you be able to create a unit test that would reproduce the error? Or just share some code to reproduce the error? Thanks.
I will put together a test. This requires a little bit of extra work because I believe a simple await of Task.Delay won't actually duplicate the issue.
Linking to a forked branch that has a test that recreates the issue. https://github.com/developermj/txFileManager/blob/async-test-and-fix/ChinhDo.Transactions.FileManagerTest/FileManagerTests.cs#L661-L697
Thanks! I will take a look soon. Chinh
@developermj Sorry for the slow response. I did some some more multithreading testing on my side and your fix looks good to me. My original premise that the _enlistments dictionary must be maintained per thread was incorrect. Can you create a PR? Thanks.
Hi,
I'm not sure there has been a PR for this.
Will it be fixed ?
Hi @GuyLescalier : @developermj did provide a fix in a forked branch but he didn't create a PR. Let me go ahead and create a PR from his code. Will do that tomorrow.
Thanks @chinhdo for your quick response !
I'm not in a hurry but I might need this fix 😃 👍
@GuyLescalier : PR #29 created. Let me test this some more. Should be able to merge to master and push out a Nuget maintenance release 1.4.1 next week. Thanks
When will the PR be merged? I was also using asynchronous transaction and just found this issue.
Did some more testing and merged the fix to master.
Didn't mean to close this issue yet until it's available in a Nuget release. But now I can't seem to re-open the issue.
Will push out a minor bug fix release to Nuget later this week.