Processing data on serial queue to prevent data racing conditions

[tifroz]

Note: data racing conditions can be detected when running the code with the Thread Sanitizer turned on (Thread Sanitizer can only be activated when running on simulators at least for iOS apps)

[swisspol]

What’s the race condition here? How do you reproduce?

This change will also prevent concurrent reading and writing on the connection.

[tifroz]

The race condition occurs on the '*_totalBytesWritten += length;*' line of '*- (void)didWriteBytes:(const void*)bytes length:(NSUInteger)length {}*' I could consistently reproduce the racing condition while streaming content from GCDServer (Async headers response + Async body - this said it could be affecting other types of response handling). I tested the same scenario after implementing the change to make sure that the changes had the intended effect (thread sanitizer did not pick up anything ) reading and writing should both be done serially, though strictly speaking they should probably be using different queues (happy to make that change if you'd like) Hugo

…

On Wed, Mar 13, 2019 at 5:11 PM Pierre-Olivier Latour < ***@***.***> wrote: What’s the race condition here? How do you reproduce? This change will also prevent concurrent reading and writing on the connection. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJLX-mxThA90WeMfYuiKHzWMot4LtxRks5vWZOtgaJpZM4bxyxW> .

[swisspol]

Can you make up a test app that reproduces the bug?

…

On Wed, Mar 13, 2019 at 7:19 PM tifroz ***@***.***> wrote: The race condition occurs on the '*_totalBytesWritten += length;*' line of '*- (void)didWriteBytes:(const void*)bytes length:(NSUInteger)length {}*' I could consistently reproduce the racing condition while streaming content from GCDServer (Async headers response + Async body - this said it could be affecting other types of response handling). I tested the same scenario after implementing the change to make sure that the changes had the intended effect (thread sanitizer did not pick up anything ) reading and writing should both be done serially, though strictly speaking they should probably be using different queues (happy to make that change if you'd like) Hugo On Wed, Mar 13, 2019 at 5:11 PM Pierre-Olivier Latour < ***@***.***> wrote: > What’s the race condition here? How do you reproduce? > > This change will also prevent concurrent reading and writing on the > connection. > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #417 (comment)>, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAJLX-mxThA90WeMfYuiKHzWMot4LtxRks5vWZOtgaJpZM4bxyxW > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmUy1YF6nvpj6gEYHjRAf555RfcNkYKks5vWbGYgaJpZM4bxyxW> .

[tifroz]

In the demo ViewController.swift, replace the definition for webServer and for viewWillAppear like so.

Then GET "/" (e.g. curl http://[gcdServerAddress]:8080/)

var webServer: GCDWebServer!

  override func viewWillAppear(_ animated: Bool) {
    super.viewWillAppear(animated)

    webServer = GCDWebServer()
    webServer.delegate = self
    webServer.addHandler(forMethod: "GET", path: "/", request: GCDWebServerRequest.classForCoder()) { [weak webServer]
      (gcdRequest:GCDWebServerRequest?, completion:GCDWebServerCompletionBlock?) -> Void in

      let contentLength: Int = 10000
      DispatchQueue.main.asyncAfter(wallDeadline: .now() + 0.5, execute: {
        let localResponse = GCDWebServerStreamedResponse(contentType: "application/data", asyncStreamBlock: { (bodyReaderBlock: GCDWebServerBodyReaderCompletionBlock?) -> Void in

          for index in 0..<contentLength {
            let buffer = "a".data(using: String.Encoding.utf8)!
            bodyReaderBlock?(buffer, nil)
          }
        })
        localResponse.statusCode = 200
        completion?(localResponse)
      })
    }
    
    if webServer.start() {
      label?.text = "Server running on port \(webServer.port).\n\nGET '/' to start test"
    } else {
      label?.text = "Server failed to start :("
    }
  }

[tifroz]

The code above reproduced the issue for me without fault (with the Thread Sanitizer turned on ). Let me know if you can't reproduce / need me to make adjustments to the Pull Request?

[swisspol]

Thanks for sharing this, I'll give it a look when I get the chance.

[tifroz]

@swisspol did you get a chance to look at this? I was about to create another PR to address issues detected by the static analyzer but I don't think I can create another fork without dumping the fork that this PR (#417) is based on.

I am totally ok if you'd rather dump this PR in order to move forward.

For context, I am adding a screenshot of the output of the static analyzer (2 issues: a nil function reference, and a memory leak). The fixes are pretty simple: in GCDWebServerConnection.m line 760, wrap the function call with if (_handler != nil) {}, and in GCDWebServer.m adding CFRelease(txtData); after line 613