Move packet writing code, add graceful shutdown handler, fix close handshake #43
Conversation
Snippets/WebsocketTest.swift
Outdated
| router.ws("/ws") { inbound, outbound, _ in | ||
| for try await packet in inbound { | ||
| router.ws("/ws") { ws, _ in | ||
| for try await packet in ws.inbound { |
There was a problem hiding this comment.
I like that this follows the pattern found in NIO. But I wonder if ws.inbound as a symbol is easy to reason about for newcomers to the ecosystem.
Snippets/WebsocketTest.swift
Outdated
| if case .text("disconnect") = packet { | ||
| break | ||
| } | ||
| try await outbound.write(.custom(packet.webSocketFrame)) | ||
| try await ws.outbound.write(.custom(packet.webSocketFrame)) |
There was a problem hiding this comment.
ws.write would also be easier to reason about IMO
| try await handler(webSocketHandlerInbound, webSocketHandlerOutbound, context) | ||
| try await self.close(code: .normalClosure, outbound: outbound, context: context) | ||
| } onGracefulShutdown: { | ||
| Task { |
There was a problem hiding this comment.
Is there a way around an unstructured task here?
There was a problem hiding this comment.
Unfortunately not. I can't close either the inbound stream or outbound writer as they are needed for the close handshake. It's a recurring issue with graceful shutdown handlers, where something more complex is need to initiate shutdown.
There was a problem hiding this comment.
I could have a child task that waits on a continuation and then calls close. I then resume the continuation in the graceful shutdown. But I'd have to somehow get the continuation to the shutdown, and also support the situation where the graceful shutdown is called before the continuation is created. That'd be a bit of a mess
| guard self.type == .client else { return nil } | ||
| let bytes: [UInt8] = (0...3).map { _ in UInt8.random(in: .min ... .max) } | ||
| return WebSocketMaskingKey(bytes) | ||
| try await outbound.write(frame: .init(fin: true, opcode: .connectionClose, data: buffer)) |
There was a problem hiding this comment.
I think we should consider making close non-throwing. I get that writing a frame can fail, but that's an acceptable and ignorable failure in my book - at this point in the WebSocket's lifecycle. Possibly something to ask the NIO team's opinion on.
There was a problem hiding this comment.
I disagree here, this initiates the close handshake and doesn't actually close the websocket. It only gets closed when we receive the close connection message from the client. If we ignore the error then the close connection is never sent and we don't receive a close connection from the client. Remember also this is an internal function. Users will close the connection by exiting the handler.
In the current situation when the error is thrown out of the asyncChannel.executeThenClose closure which will force close the connection.
There was a problem hiding this comment.
Ah, good clarification. I misremember that then
2d1d5d6 to
5140f09
Compare
WebSocketOutboundWriter