TSAN error when parsing URLs

[shadowfacts]

I was running with the thread sanitizer enabled to test something else, and ran into a crash in WebURL's parsing code. It happens at this line: https://github.com/karwa/swift-url/blob/main/Sources/WebURL/Parser/Parser.swift#L74 because the closures passed in as the left/right params to map both try to capture mutable references to callback with the ampersand operator.

Even though left and right are never both called, this is still an exclusivity violation:

TSAN is trying to be helpful by checking for simultaneous inout operations on the property, even if we never perform an actual memory access. Simply accessing the property with & triggers the race. What's more it is technically Swift's right to crash with an exclusivity error, though it doesn't happen to do so as of this writing. http://www.russbishop.net/the-law

The solution, I think, is to just lift the switch in Either.map out into _urlFromBytes_impl so that the two closures don't need to be created.

[karwa]

Hmm, referring to a variable in a non-escaping closure is, AFAIK, not a formal access and so should not cause an exclusivity violation. The formal access is around the actual function call within the closure, so lifting the switch in to the body of the function shouldn't make a difference. I tried, I was still able to reproduce the issue:

I actually think this could be a bug in TSan. If it really was an exclusivity violation, I would expect that it would trigger every time - and yet, even though I am able to reproduce it, it doesn't happen every time. It occurs when called during one of the Foundation tests, but in every other test (including the ones which parse thousands of URLs), it doesn't trigger at all. So that's immediately suspicious. I've tried XCode 13.4.1, 14 RC, and a recent 5.7 nightly toolchain, and it's not reliable on any of them.

What is also suspicious is that the message says something about the function being a global variable (?!), and that the race involves mutating accesses from multiple threads. It just doesn't make any sense.

Still, it would help to know:

1. Which version of XCode are you using?

2. If you try a small, self-contained example - just a simple package with code like this:

   import WebURL
   import Dispatch
   
   DispatchQueue.concurrentPerform(iterations: 100_000) { num in
     WebURL("https://test/a/b/\(num)")!
   }
   

   And run it with swift run --sanitize=thread, do you see any reports? What about if you do a non-concurrent loop?

   for num in 0..<100_000 {
     WebURL("https://test/a/b/\(num)")!
   }
   

[shadowfacts]

I'm using the Xcode 14 RC.

Oddly, I'm not longer able to reproduce the outright crash in my app and I'm getting (almost) the same runtime warning you're seeing. When I reported this yesterday, the crash was 100% repro-able (and hand-inling the map function fixed it).

This is the warning I'm seeing (and it only reports an access from a single thread):

But, using this inside of an XCTestCase in a Swift package that depends on WebURL (being run from an xcodeproj that includes the package):

func testWebURL() {
    DispatchQueue.concurrentPerform(iterations: 100_000) { i in
        _ = WebURL("https://example.com/foo/\(i)")
    }
}

I am able to reproduce the crash consistently when running the test with the thread sanitizer enabled in the Test scheme configuration.

Before the crash, I sometimes (not consistently) get the same runtime sanitizer warning and two threads are reported accessing the same variable (although the report printing in the console says the variable being access is at 0x780000000000 which seems like a suspiciously round number).

Using the same code, but in the main function of a standalone Swift package, I'm not able to reproduce either the warning or the crash when running with --sanitize=thread.

[karwa]

OK, so this very much looks like a bug in TSan.

I have a branch with a fix, tsan-workaround (PR #168). Would you be able to try it and confirm that it fixes the issue for you?

[shadowfacts]

That branch does indeed fix the issue.

[karwa]

Should be fixed in 0.4.1. Feel free to reopen if it is not fixed.