search

chinhdo / txFileManager

.NET Transactional File Manager is a .NET Standard library that allows you to enlist file operations (file/folder copies, writes, deletes, appends, etc.) in distributed transactions.
MIT License
111 stars 13 forks source link

NullReferenceException with async code #27

Closed developermj closed 3 years ago

developermj commented 3 years ago

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.

chinhdo commented 3 years ago

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.

developermj commented 3 years ago

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.

developermj commented 3 years ago

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

chinhdo commented 3 years ago

Thanks! I will take a look soon. Chinh

chinhdo commented 3 years ago

@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.

GuyLescalier commented 3 years ago

Hi, I'm not sure there has been a PR for this. Will it be fixed ?

chinhdo commented 3 years ago

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.

GuyLescalier commented 3 years ago

Thanks @chinhdo for your quick response ! I'm not in a hurry but I might need this fix 😃 👍

chinhdo commented 3 years ago

@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

LoTT97 commented 3 years ago

When will the PR be merged? I was also using asynchronous transaction and just found this issue.

chinhdo commented 3 years ago

@LoTT97 : Will do some more tests and complete the PR this week.

chinhdo commented 3 years ago

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.

chinhdo commented 3 years ago

Published version 1.5 to Nuget.