[WIP] Testing fixing approaches for tsan RxAtomic issues

[LucianoPAlmeida]

Fixes #1853.

[freak4pc]

Very cool !!

[freak4pc]

@andyj-at-aspin
Can you let us know if this branch solves the issues you were having ?

[andyj-at-aspin]

Sadly the above commits don't do it for me. Remember, I've included RxSwift as a git submodule and sub-project, this leaves the source code for Platform/AtomicInt.swift exposed to Xcode and it's that falls down.

[freak4pc]

Could it be that increment itself needs to be marked that way @LucianoPAlmeida ?

[freak4pc]

Also, @andyj-at-aspin - if you could provide a project reproducing that, it might be easier to debug.

[kzaher]

Hi @LucianoPAlmeida ,

This looked like a really promising approach, but unfortunately after testing it it doesn't seem to fix the issue. After turning on the thread sanitizer for the unit tests, failures are still reported for atomic operations, although the tests itself pass without it :(

[LucianoPAlmeida]

@freak4pc Now that you point out, I have a guess on what is happening :))
Since AtomicInt is a swift struct and increment is a mutating function, self is implicity passed to AtomicInt increment function as an inout param and on Swift mutating functions self have write access (requires exclusive access) that automatically starts at the beginning of the function and ends when the function finishes. So the thread sanitizer is not complaining about RxAtomic C functions, but is complaining about those write access on self in the mutating function increment and decrement. So it basically requires the lock to be made by the caller(I think that is why it doesn't work). RxAtomic add and load function are fine, the actual problem is on the swift mutating functions exclusive access.
Not sure if I could explain that right but I wrote a blog post on that exact subject here
A lot more details there :))
There are a few approaches to fix it, I'll try then asp

• Find a way to making AtomicInt a class so self will be passed as a ref and not require exclusive access on.
• Lock around the AtomicInt calls on Resources methods.

I'm quite sure that this is the issue, but will run some tests and let you guys know :))

[RxPullRequestBot]

	2 Warnings
⚠️	PR is classed as Work in Progress
⚠️	No CHANGELOG changes made

Generated by 🚫 Danger

[LucianoPAlmeida]

Put it on WIP, testing another approach that seems to fix it.
But having trouble with SPM and Linux build :((
This approach is just to remove the atomic operations of mutating functions (functions that have exclusive access rules) and make then static functions and abstracting them into a Box class. I think that solves the issue, but running the unit tests with TSan it seems to hit other issues on other places, I think is not related to that, but still, a Tsan issue, had to confirm it :))
@kzaher Can you take a look on with this branch to confirm if those are if fact unrelated issues?
@andyj-at-aspin Can you do tests with this branch and let us know if it solves the issues you were having?
I'll try do run more tests as soon as possible.

[andyj-at-aspin]

Well this branch now works fine without any TSAN reports for my project. The boxing into a class satisfies the issue I had already though I'd explained that it was the mutability of the swift struct that was at issue, not the locking mechanisms. I only didn't mention it because I saw you guys were so keen on performance and I didn't know if moving to a class would effect that.

I'm a bit worried that the CI failures from Travis mean this branch somehow has a fatal flaw but that's beyond by understanding.

[LucianoPAlmeida]

@andyj-at-aspin Thank's for the test :)) I'm still trying to figure out some things to make this pass CI, but I think is just files imported and some script validations.

[kzaher]

@LucianoPAlmeida I've fixed some issues here https://github.com/ReactiveX/RxSwift/tree/LucianoPAlmeida-no-sanitize-thread-atomic

We can probably remove AtomicInt member methods completely and just use RxAtomic.AtomicInt and static methods everywhere.

[freak4pc]

Looks great!

[LucianoPAlmeida]

@kzaher Awesome :))
Notice that you already remove those usages making them arbitrary functions 👍
Should we close this one and open the PR for that branch?
There is something more to do on that? Besides for the spec validation issues, it seems fine