twitter-archive / CocoaSPDY

SPDY for iOS and OS X
Apache License 2.0
2.39k stars 233 forks source link

ignore setLoggerLevel: when compiling w/thread-sanitizer on #163

Open johnkdoe opened 7 years ago

johnkdoe commented 7 years ago

during unit-testing with the thread-sanitizer on, setLoggerLevel: was flagged unnecessarily as a potential data-race.

__sharedLoggerLevel is already marked volatile, and the location of the data race indicated was a debug logging message .

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 76.243% when pulling d6c03e1fd50c7cd73c2ca1012494ae5f84f7fa8a on johnkdoe:develop-with-thread-sanitizer into 065fb0feaf0094bc9bf2eaf7afe8d2f4279be3fe on twitter:develop.

NSProgrammer commented 7 years ago

What unit tests? How can I repro? The move to stdatomic SHOULD have solved this and for thread sanitizer to still elicit warnings makes me want to double check that we are properly accessing the variable everywhere.

kgoodier commented 7 years ago

I did some research and volatile is not necessary with the std::atomic variables, of which __sharedLoggerLevel is one. We should probably skip this change, and just remove the volatile keyword. I'm happy to do that, but I don't know how to repro this sanitizer error you're seeing; @johnkdoe can you try that change and see if stops?

UPDATE: std::atomic is for C++, which is not what we are using here. We are using , the C version, which uses the _Atomic keyword. It is much more unclear about the use of the volatile keyword, but seems to be independent of it. They can be used together or independently. I still say we don't need the 'volatile' keyword, but it's not hurting anything.

johnkdoe commented 7 years ago

for Twitter, was getting this just running the "Bluebird Enterprise" scheme, performing unit tests with thread sanitizer turned on. can try to re-test with __sharedLoggerLevel established as a non-volatile …

johnkdoe commented 7 years ago

deleted last 2 comments with stack-traces of thread-sanitizer … and also got the latest proper SHA1 for the develop branch …

kgoodier commented 7 years ago

I guess the thread sanitizer is technically correct, __sharedLoggerLevel is, by design, a race between writes & reads. It doesn't matter though, as long as it is updated atomically. I'm surprised the thread sanitizer isn't more cognizant or accepting of usage patterns like this. I suppose the only thing we can do is disable it the check, as you've proposed in this request.

CLAassistant commented 5 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Kirk Beitz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.