issue #318: Address warning reported by thread safety analyzer #319
Conversation
|
@Lukasa Could you please review and guide. I am not sure that we can have tests to accompany the change and I am not sure that I can fully defend that |
| // we materialize Values into an Array before returning from under a lock | ||
| // as thread safety analyzer considers safety of Values as undefined and dependant on implementation | ||
| // e.g. a lazy collection implementation could by thread unsafe to be returned from within a lock | ||
| Array(self.providers.values) |
There was a problem hiding this comment.
I do not believe this fix is necessary: I think TSAN is wrong.
There was a problem hiding this comment.
I am lacking deep knowledge of Swift runtime / libraries here :(.
Is it possible that @ktoso could advise here?
I seen in person issues with returning a lazy collection out of lock guarded block in Scala many years ago and performing of "materialise" was a go-to tool then.
There was a problem hiding this comment.
While the dictionary values view is "lazy", conceptually, it is nonetheless still a value type with value semantics. That means that changes to the dictionary to which it points must not manifest in changes to this value. Acquiring the view must be done with a lock, but once it's copied out we're safe. Either TSAN is wrong, or Dictionary in Swift 5.3 is implemented incorrectly.
There was a problem hiding this comment.
PR is updated to contain only .count fix according to the discussion above.
Motivation: Thread safety analyzer reports warning about observable state leaving lock guarded code blocks. This PR address some of the warnings. Analysis of the warning does not prove that we have a real bug and all the warnings are considered as potential bugs. Analyzer used is: `docker run --rm -v $(pwd):/src -w /src swift:5.3.1 swift test -c release --sanitize=thread --enable-test-discovery` Modifications: * accessor to count of connections in connection pool is guarded by lock Result: Most of thread safety warnings are addressed without modification of observable code behaviour.
ya-pulser
force-pushed
the
issue_318_cover_with_lock
branch
from
3e5c3dc to
87b87fa
Compare
Motivation:
Thread safety analyzer reports warning about observable state
leaving lock guarded code blocks. This PR address the warnings.
Analysis of the warning does not prove that we have a real bug and all the
warnings are considered as potential bugs.
Analyzer used is:
docker run --rm -v $(pwd):/src -w /src swift:5.3.1 swift test -c release --sanitize=thread --enable-test-discoveryModifications:
the collection from lock guarded code block
Result:
Most of thread safety warnings are addressed without modification of observable
code behaviour.