-
Notifications
You must be signed in to change notification settings - Fork 719
Allow to specify the max websockets frame size when using the upgrader. #315
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
Changes from all commits
97d0cd0
aee563f
b686693
b9036b0
d679b0b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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`. | ||
| /// | ||
|
|
@@ -73,10 +74,44 @@ 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. | ||
| public init(shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?, | ||
| /// - 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 convenience init(shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?, | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>) { | ||
| self.init(maxFrameSize: 1 << 14, shouldUpgrade: shouldUpgrade, upgradePipelineHandler: upgradePipelineHandler) | ||
| } | ||
|
|
||
|
|
||
| /// Create a new `WebSocketUpgrader`. | ||
| /// | ||
| /// - parameters: | ||
| /// - 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. Users may set this to any value up to `UInt32.max`. | ||
| /// - shouldUpgrade: A callback that determines whether the websocket request should be | ||
| /// upgraded. This callback is responsible for creating a `HTTPHeaders` object with | ||
| /// any headers that it needs on the response *except for* the `Upgrade`, `Connection`, | ||
| /// and `Sec-WebSocket-Accept` headers, which this upgrader will handle. Should return | ||
| /// `nil` if the upgrade should be refused. | ||
| /// - upgradePipelineHandler: A function that will be called once the upgrade response is | ||
| /// flushed, and that is expected to mutate the `Channel` appropriately to handle the | ||
| /// websocket protocol. This only needs to add the user handlers: the | ||
| /// `WebSocketFrameEncoder` and `WebSocketFrameDecoder` will have been added to the | ||
| /// pipeline automatically. | ||
| public init(maxFrameSize: Int, shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?, | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>) { | ||
| precondition(maxFrameSize <= UInt32.max, "invalid overlarge max frame size") | ||
| self.shouldUpgrade = shouldUpgrade | ||
| self.upgradePipelineHandler = upgradePipelineHandler | ||
| self.maxFrameSize = maxFrameSize | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
|
@@ -111,7 +146,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) | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it make even more sense to test behavior (e.g., ability and inability to transfer more than the limit when the limit is appropriately changed) rather than checking whether we set the
maxFrameSize. It'll have a nice side effect of eliminating this comment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vlm I will add more tests as a followup to test max frame size handling. But I think this is ok to test what the change did.