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

Allow to specify the max websockets frame size when using the upgrader. #315

Merged
merged 5 commits into from
Apr 16, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/NIOWebSocket/WebSocketFrameDecoder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public final class WebSocketFrameDecoder: ByteToMessageDecoder {
public var cumulationBuffer: ByteBuffer? = nil

/// The maximum frame size the decoder is willing to tolerate from the remote peer.
private let maxFrameSize: Int
public let maxFrameSize: Int
Copy link
Member Author

Choose a reason for hiding this comment

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

we can also keep this internal if we like

Copy link
Member

Choose a reason for hiding this comment

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

my vote would go to let's keep it internal :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I think we should keep it internal too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's always easier to make things more public than to make them less public.


/// Our parser state.
private var parser = WSParser()
Expand Down
16 changes: 14 additions & 2 deletions Sources/NIOWebSocket/WebSocketUpgrader.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public final class WebSocketUpgrader: HTTPProtocolUpgrader {

private let shouldUpgrade: (HTTPRequestHead) -> HTTPHeaders?
private let upgradePipelineHandler: (Channel, HTTPRequestHead) -> EventLoopFuture<Void>
private let maxFrameSize: Int

/// Create a new `WebSocketUpgrader`.
///
Expand All @@ -73,10 +74,21 @@ public final class WebSocketUpgrader: HTTPProtocolUpgrader {
/// websocket protocol. This only needs to add the user handlers: the
/// `WebSocketFrameEncoder` and `WebSocketFrameDecoder` will have been added to the
/// pipeline automatically.
/// - maxFrameSize: The maximum frame size the decoder is willing to tolerate from the
/// remote peer. WebSockets in principle allows frame sizes up to `2**64` bytes, but
/// this is an objectively unreasonable maximum value (on AMD64 systems it is not
/// possible to even allocate a buffer large enough to handle this size), so we
/// set a lower one. The default value is the same as the default HTTP/2 max frame
/// size, `2**14` bytes. Users may override this to any value up to `UInt32.max`.
/// Users are strongly encouraged not to increase this value unless they absolutely
/// must, as the decoder will not produce partial frames, meaning that it will hold
/// on to data until the *entire* body is received.
public init(shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?,
upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>) {
upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>, maxFrameSize: Int = 1 << 14) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I recommend that instead of making this change you just add a new constructor that this one calls, with maxFrameSize earlier in the argument list? Trailing closures are really nice, and it's a shame to lose them.

precondition(maxFrameSize <= UInt32.max, "invalid overlarge max frame size")
Copy link
Member

Choose a reason for hiding this comment

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

if you're already preconditioning here, mind checking it's positive too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any point in this precondition? I think it just duplicates logic that exists in the web socket frame decoder.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Lukasa imho its better to fail fast... WDYT ?

self.shouldUpgrade = shouldUpgrade
self.upgradePipelineHandler = upgradePipelineHandler
self.maxFrameSize = maxFrameSize
}
Copy link
Contributor

@vlm vlm Apr 16, 2018

Choose a reason for hiding this comment

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

Curious, what is the motivation for providing two public constructors rather than a constructor with the default value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@vlm Short answer is "to preserve the trailing closure".


public func buildUpgradeResponse(upgradeRequest: HTTPRequestHead, initialResponseHeaders: HTTPHeaders) throws -> HTTPHeaders {
Expand Down Expand Up @@ -111,7 +123,7 @@ public final class WebSocketUpgrader: HTTPProtocolUpgrader {

public func upgrade(ctx: ChannelHandlerContext, upgradeRequest: HTTPRequestHead) -> EventLoopFuture<Void> {
return ctx.pipeline.add(handler: WebSocketFrameEncoder()).then {
ctx.pipeline.add(handler: WebSocketFrameDecoder())
ctx.pipeline.add(handler: WebSocketFrameDecoder(maxFrameSize: self.maxFrameSize))
}.then {
self.upgradePipelineHandler(ctx.channel, upgradeRequest)
}
Expand Down
1 change: 1 addition & 0 deletions Tests/NIOWebSocketTests/EndToEndTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ extension EndToEndTests {
("testUpgradeMayAddCustomHeaders", testUpgradeMayAddCustomHeaders),
("testMayRegisterMultipleWebSocketEndpoints", testMayRegisterMultipleWebSocketEndpoints),
("testSendAFewFrames", testSendAFewFrames),
("testMaxFrameSize", testMaxFrameSize),
]
}
}
Expand Down
25 changes: 25 additions & 0 deletions Tests/NIOWebSocketTests/EndToEndTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -340,4 +340,29 @@ class EndToEndTests: XCTestCase {

XCTAssertEqual(recorder.frames, [dataFrame, pingFrame])
}

func testMaxFrameSize() throws {
let basicUpgrader = WebSocketUpgrader(shouldUpgrade: { head in HTTPHeaders() },
upgradePipelineHandler: { (channel, req) in
return channel.eventLoop.newSucceededFuture(result: ())
}, maxFrameSize: 16)
let (loop, server, client) = createTestFixtures(upgraders: [basicUpgrader])
defer {
XCTAssertNoThrow(try client.finish())
XCTAssertNoThrow(try server.finish())
XCTAssertNoThrow(try loop.syncShutdownGracefully())
}

let upgradeRequest = self.upgradeRequest(extraHeaders: ["Sec-WebSocket-Version": "13", "Sec-WebSocket-Key": "AQIDBAUGBwgJCgsMDQ4PEC=="])
XCTAssertNoThrow(try client.writeString(upgradeRequest).wait())
interactInMemory(client, server)

let receivedResponse = client.readAllInboundBuffers().allAsString()
assertResponseIs(response: receivedResponse,
expectedResponseLine: "HTTP/1.1 101 Switching Protocols",
expectedResponseHeaders: ["Upgrade: websocket", "Sec-WebSocket-Accept: OfS0wDaT5NoxF2gqm7Zj2YtetzM=", "Connection: upgrade"])

let decoder = (try server.pipeline.context(handlerType: WebSocketFrameDecoder.self).wait()).handler as! WebSocketFrameDecoder
XCTAssertEqual(16, decoder.maxFrameSize)
}
}