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

Sendable Crash even on 2.13.0 / Swift 5.7 #139

Closed
PatrickPijnappel opened this issue May 26, 2023 · 6 comments
Closed

Sendable Crash even on 2.13.0 / Swift 5.7 #139

PatrickPijnappel opened this issue May 26, 2023 · 6 comments
Labels
bug Something isn't working

Comments

@PatrickPijnappel
Copy link

PatrickPijnappel commented May 26, 2023

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

  • Vapor Framework version: main/99209351d4a1d92d92cbdc844c49b6fa5a28cc58
  • Vapor Toolbox version:
  • OS version: Ubuntu 20.04.4 LTS (GNU/Linux 5.15.0-1036-aws x86_64)
@PatrickPijnappel PatrickPijnappel added the bug Something isn't working label May 26, 2023
@0xTim
Copy link
Member

0xTim commented May 26, 2023

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

@PatrickPijnappel
Copy link
Author

PatrickPijnappel commented May 26, 2023

@0xTim Basically this:

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

@0xTim
Copy link
Member

0xTim commented May 26, 2023

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

@PatrickPijnappel
Copy link
Author

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
Copy link
Member

0xTim commented May 29, 2023

@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
Copy link
Author

@0xTim Ah i see, thank you!

@0xTim 0xTim closed this as completed May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants