vapor / websocket-kit

WebSocket client library built on SwiftNIO
https://docs.vapor.codes/4.0/advanced/websockets/
MIT License
272 stars 79 forks source link

Sendable Crash even on 2.13.0 / Swift 5.7 #139

Closed PatrickPijnappel closed 1 year ago

PatrickPijnappel commented 1 year ago

Describe the bug

We're getting this crash with 2.12.0, then it was fixed with 2.12.1 but failing again with 2.13.0 (Swift 5.7)

0x561dd35a22d0, Swift runtime failure: precondition failure at /home/ubuntu/CodeNew/<compiler-generated>:0
0x561dd35a22d0, function signature specialization <Arg[0] = Dead, Arg[1] = Dead> of (extension in NIOCore):NIOCore.EventLoop.preconditionInEventLoop(file: Swift.StaticString, line: Swift.UInt) -> () at /home/ubuntu/CodeNew/<compiler-generated>:0
0x561dd35a22d0, (extension in NIOCore):NIOCore.EventLoop.preconditionInEventLoop(file: Swift.StaticString, line: Swift.UInt) -> () at /home/ubuntu/CodeNew/<compiler-generated>:0
0x561dd35a22d0, generic specialization <@Sendable (WebSocketKit.WebSocket, NIOCore.ByteBuffer) -> ()> of NIOCore.NIOLoopBoundBox.value.setter : A at /home/ubuntu/CodeNew/<compiler-generated>:0
0x561dd35a22d0, WebSocketKit.WebSocket.onBinary((WebSocketKit.WebSocket, NIOCore.ByteBuffer) -> ()) -> () at /home/ubuntu/CodeNew/.build/checkouts/websocket-kit/Sources/WebSocketKit/WebSocket.swift:66
0x561dd32897f2, closure #1 () -> () in closure #1 (Any) -> () in closure #1 (Any) -> () in Server.ComputeInstance.(getWebSocket in _125ACE48D80A84E46C4244E72A933F8D)(app: Vapor.Application) -> Utilities.Promise<WebSocketKit.WebSocket> at /home/ubuntu/CodeNew/Sources/Server/Instances/ComputeInstance.swift:123
0x561dd274f695, reabstraction thunk helper from @escaping @callee_guaranteed () -> () to @escaping @callee_unowned @convention(block) () -> () at /home/ubuntu/CodeNew/<compiler-generated>:0

To Reproduce

(Not sure at this point)

Expected behavior

No crash.

Environment

0xTim commented 1 year ago

@PatrickPijnappel what does your web socket code look like? We're probably missing a hop somewhere

PatrickPijnappel commented 1 year ago

@0xTim Basically this:

self.queue.async {
    // (…)
    webSocket.onBinary { [weak self] _, data in // Called from arbitrary queue
        guard let self = self else { return }
        // (…)
    }
}
0xTim commented 1 year ago

What is self in this instance and queue? Im assuming the web socket is being created outside of the queue closure?

PatrickPijnappel commented 1 year ago

self is a custom (final) class and queue is a serial DispatchQueue. The web socket is being created elsewhere on another queue using effectively

WebSocket.connect(to: url, on: app.eventLoopGroup.next()) { webSocket in
    queue.async { self.webSocket = webSocket }
}
0xTim commented 1 year ago

@PatrickPijnappel ah ok, I misread this initially.

Setting the WS callbacks must be done on the Websocket's EventLoop. We assumed this was the case before but had no way to enforce this at a compiler level before Sendable. Doing it off the event loop might have worked but could easily caused crashes before and for us to be Sendable compliant we have to enforce it.

One workaround might be to wrap your calls in

websocket.eventLoop.execute {
  websocket.onBinary {
    // ...
  }
}

That should enforce that the mutations are done on the correct event loop, but mixing concurrency, event loops and dispatch queues like this is always going to cause issues

PatrickPijnappel commented 1 year ago

@0xTim Ah i see, thank you!