Skip to content
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 and Possible race conditions #27

Open
theodoreOnzGit opened this issue Jul 27, 2022 · 4 comments
Open

Thread safety and Possible race conditions #27

theodoreOnzGit opened this issue Jul 27, 2022 · 4 comments

Comments

@theodoreOnzGit
Copy link

theodoreOnzGit commented Jul 27, 2022

Hello, thanks for providing such a wonderful piece of software.

Ive been writing code based on your modules and for some reason when i use xunit, there happens to be unrepeatable wrong unit exceptions which come. It means like once every twenty or thirty test runs, a wrong unit exception comes up.

When i rerun the tests, it disaappears and the tests pass.

It's about 200+ tests, and i strongly suspect it's because xunit does things in parallel, and also because some classes use static operators eg, addition and subtraction.

I strongly suspect a race condition as several classes or test cases are accessing static operators at the same time. And therefore it's not really thread safe.

Were there any of such cases before?

And what are some possible fixes we can work towards?

Edit: I'll also be checking in case my code (or what i'm building off - spicesharp) isn't thread safe. But tbh i have never used static methods in my code and all the exceptions and errors i've gotten were from the EngineeringUnits package tho.

@MadsKirkFoged
Copy link
Owner

We worked out a thread safety bug in #17 so make sure you are using the latest version.

Can you create a test case where you run your code in parallel and try to spam it to see if you get any race conditions?
I have tried myself but I could not find any race conditions

@theodoreOnzGit
Copy link
Author

Hey MadsKirkFoged,

Thanks for working on the SharpFluids bit with me.

I know I should be doing this so it makes it easier for you to fix.
Unfortunately, I myself cannot nail down the exact lines of code at the moment.
So i cannot write a test case until then. And searching for something like that is a needle in a haystack;

I've been using XUnit to unit test my new code libraries. And it is depdendent on EngineeringUnits, SharpFluids
as well as the spiceSharp nuget package. I was incorporating engineering units into the spiceSharp code
which is an electrical circuit solver and hopefully extending it to fluid flow.

So it could be the spiceSharp Code that's not thread safe, or it could be yours? I don't know. And the exceptions are very hard to catch... But it seems everytime there is an exception, it's thrown by the EngineeringUnits package.

Anyhow, i don't want to let you do more work than you need to. If i am able to pinpoint the case and run XUnit tests, I'll find it for you and write the code here.

It's like weird, the moment i add features in my code which increase calculation time, all of a sudden i don't see the exceptions being thrown anymore.

@MadsKirkFoged
Copy link
Owner

Im pretty sure the CoolProp dll is not Thread safe and since SharpFluids relays on it, then SharpFluids is not Thread safe either!
Except rewriting the CoolProp dll in C# I haven't found a good solution for that.

If you have a test that fails that is not using SharpFluids, that would be interesting to know about

@theodoreOnzGit
Copy link
Author

Okay sure, I'll let you know if anything crops up

So sad though, i was intending to use sharpfluids for parallel computation (ahhhhh, #sadface)

But as you said, coolprop dlls are not natively threadsafe

Just inserting this here in case anyone else is reading the thread
CoolProp/CoolProp#1456
https://stackoverflow.com/questions/60872766/parallel-invocations-of-c-library-wrapped-with-pinvoke-raises-system-accessvio

Based on these though, I've seen the use of AbstractState in C++ which helps achieve threadsafety
http://www.coolprop.org/_static/doxygen/html/class_cool_prop_1_1_abstract_state.html

Basically for anyone else reading abstract classes are C++ version's of interfaces. (C# has abstract classes also, but interfaces are distinct)

This means one has got to move into coolprop via C++, code and compile a thread safe version of the library, and then generate the dlls.

Yikes

I might revisit this if i really need the speed, I hope i don't =(
Will let you know if i find anything. And if i can make it thread safe.

This is such a challenging issue lol.

But again okay, i'll let you know if anything. I'm just thinking out loud here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants