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

WebSocket data frame streaming #53

Merged
merged 11 commits into from
Apr 8, 2024
Merged

WebSocket data frame streaming #53

merged 11 commits into from
Apr 8, 2024

Conversation

adam-fowler
Copy link
Member

Adding methods to stream WebSocket messages inbound and outbound.

Inbound

By default WebSocketInboundStream is now a stream of fragmented WebSocket data frames. You can convert this to a stream of WebSocket messages using WebSocketInboundStream.messages(maxSize:). eg

for try await message in inbound.messages(maxSize: 1024) {
    ...
}

You can also extract a single complete message from an inbound iterator using

var inboundIterator = inbound.makeAsyncIterator()
let msg = try await inboundIterator.nextMessage(maxSize: 1024)

Outbound

Two new functions have been added to WebSocketOutboundWriter: withTextMessageWriter(:_) and withBinaryMessageWriter(:_). With these you can write a single message across fragmented frames.

try await outbound.withTextMessageWriter { writer in
    try await writer("Merry Christmas ")
    try await writer("and a ")
    try await writer("Happy new year")
}

@adam-fowler adam-fowler requested a review from Joannis April 8, 2024 06:51
for try await packet in inbound {
if case .text("disconnect") = packet {
for try await frame in inbound {
if frame.opcode == .text, String(buffer: frame.data) == "disconnect", frame.fin == true {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot, since we can now also decode a frame.opcode == .text into JSON without having to do the dance of converting to String inbetween. Because currently, we'd need to convert ByteBuffer into String, then into Data again.

Copy link
Member Author

@adam-fowler adam-fowler Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is like this because a single frame fragment can have incomplete UTF8 data, so we can't decode it until we have all the frames that make up a message.

Unfortunately the WebSocketMessage text case is a String. Should I consider changing that back to a ByteBuffer.

Sources/HummingbirdWebSocket/WebSocketOutboundWriter.swift Outdated Show resolved Hide resolved

/// Write a single WebSocket binary message as a series of fragmented frames
/// - Parameter write: Function writing frames
public func withBinaryMessageWriter<Value>(_ write: ((ByteBuffer) async throws -> Void) async throws -> Value) async throws -> Value {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could leverage the same writer mentioned above, but the writer would store the expected opcode. I think it's valid to send a String over a binary WebSocket message.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@tkrajacic tkrajacic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it

}

func processFrameToSend(_ frame: WebSocketFrame, context: some WebSocketContext) async throws -> WebSocketFrame {
let isCorrectType = frame.opcode == .text || frame.opcode == .binary
let isCorrectType = frame.opcode == .text || frame.opcode == .binary || frame.opcode == .continuation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% certain this logic is correct.
frame.opcode == .continuation might be set for a message with a yet unknown (non-control-frame) opcode, that is not text or binary and defined in an extension. The check for text or binary is somewhat incompatible with this condition, as now we include continuations of other types (excluding their initial frames)

See https://www.rfc-editor.org/rfc/rfc6455.html#section-5.4

Wouldn't we need to keep track of the opcode that the current continuation pertains to?

Then again… one could argue that we don't implement the ability to handle any other opcodes, and would error out earlier, so this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any particular unknown opcodes that have associated application data? If not then the only ones known are binary, text and continuation. And a continuation only appears once we have received a data or text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deflate doesn't care if a frame is text or binary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, you are right. we need to close the connection anyway if there are unknown opcodes.

Sources/HummingbirdWebSocket/WebSocketFrameSequence.swift Outdated Show resolved Hide resolved
Sources/HummingbirdWebSocket/WebSocketInboundStream.swift Outdated Show resolved Hide resolved
@adam-fowler adam-fowler merged commit 0016b8f into main Apr 8, 2024
3 of 4 checks passed
@adam-fowler adam-fowler deleted the max-message-size branch April 8, 2024 16:36
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.

3 participants