Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race on connected #355

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Conversation

danielrhammond
Copy link
Contributor

doDisconnect(_:) gets called from a background thread as part of the operation queued in dequeueWrite(_,code:completion:) when a connection failure triggers and there's no synchronization between its setting of connected and the several places the mainthread reads its value through isConnected.

Here's an example thread sanitizer report I got by killing the server while a test app was connected.

==================
WARNING: ThreadSanitizer: data race (pid=15356)
  Write of size 1 at 0x7b5800002348 by thread T12:
  * #0 WebSocket.connected.setter WebSocket.swift (Starscream:x86_64+0xc3d5)
    #1 WebSocket.doDisconnect(_:) WebSocket.swift:930 (Starscream:x86_64+0x2f363)
    #2 closure #1 in WebSocket.dequeueWrite(_:code:writeCompletion:) WebSocket.swift:904 (Starscream:x86_64+0x2e98e)
    #3 partial apply for closure #1 in WebSocket.dequeueWrite(_:code:writeCompletion:) WebSocket.swift (Starscream:x86_64+0x3ada4)
    #4 thunk for @callee_owned () -> () WebSocket.swift (Starscream:x86_64+0x739c)
    #5 __NSBLOCKOPERATION_IS_CALLING_OUT_TO_A_BLOCK__ <null>:6963056 (Foundation:x86_64+0x441a8)
    #6 _dispatch_client_callout <null>:6963056 (libdispatch.dylib:x86_64+0x178b)

  Previous read of size 1 at 0x7b5800002348 by thread T2:
  * #0 WebSocket.connected.getter WebSocket.swift (Starscream:x86_64+0xc354)
    #1 WebSocket.isConnected.getter WebSocket.swift:136 (Starscream:x86_64+0xbd46)
    #2 WebSocket.write(string:completion:) WebSocket.swift:231 (Starscream:x86_64+0x1123b)
    #3 Controller.sendTimestamp() ViewController.swift:19 (Starscream Crash Test:x86_64+0x100002cec)
    #4 closure #1 in Controller.websocketDidConnect(socket:) ViewController.swift:28 (Starscream Crash Test:x86_64+0x1000032b3)
    #5 partial apply for closure #1 in Controller.websocketDidConnect(socket:) ViewController.swift (Starscream Crash Test:x86_64+0x10000331d)
    #6 thunk for @callee_owned () -> () ViewController.swift (Starscream Crash Test:x86_64+0x1000033ac)
    #7 __tsan::invoke_and_release_block(void*) <null>:6963056 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x6565b)
    #8 _dispatch_client_callout <null>:6963056 (libdispatch.dylib:x86_64+0x178b)

  Issue is caused by frames marked with "*".

  Location is heap block of size 664 at 0x7b5800002100 allocated by main thread:
    #0 calloc <null>:6963088 (libclang_rt.tsan_osx_dynamic.dylib:x86_64h+0x481b2)
    #1 class_createInstance <null>:6963088 (libobjc.A.dylib:x86_64h+0x6e9c)
    #2 Controller.init() ViewController.swift:8 (Starscream Crash Test:x86_64+0x100002459)
    #3 Controller.__allocating_init() ViewController.swift (Starscream Crash Test:x86_64+0x100001fb5)
    #4 ViewController.viewDidLoad() ViewController.swift:55 (Starscream Crash Test:x86_64+0x10000471f)
    #5 @objc ViewController.viewDidLoad() ViewController.swift (Starscream Crash Test:x86_64+0x100004a30)
    #6 -[NSViewController _sendViewDidLoad] <null>:6963088 (AppKit:x86_64+0x961ca)
    #7 start <null>:6963088 (libdyld.dylib:x86_64+0x5234)

  Thread T12 (tid=344393, running) is a GCD worker thread

  Thread T2 (tid=344370, running) is a GCD worker thread

SUMMARY: ThreadSanitizer: data race WebSocket.swift in WebSocket.connected.setter
==================

doDisconnect(_:) gets called from a background thread as part of the operation queued in dequeueWrite(_,code:completion:) when a connection failure triggers this code it produces a race between all the reads of isConnected and that write on a different queue.
@daltoniam
Copy link
Owner

Thanks! This has been a problem for a some time, hadn't gotten around to fixing it.

@daltoniam daltoniam merged commit 9281651 into daltoniam:master Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants