Allow to specify the max websockets frame size when using the upgrader. #315
Conversation
Motivation: At the moment its not possible to adjust the max websockets frame size when using the upgrader while its possible when directly use the WebSocketFrameDecoder. Modifications: Allow to specify max frame size via an init argument (with default value). Result: More flexible way to use the upgrader.
|
also @vlm |
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Mind writing a test for me please?
normanmaurer
force-pushed
the
websocket_upgrade_frame_size
branch
from
be08eaa to
aee563f
Compare
|
@Lukasa done |
|
|
||
| /// The maximum frame size the decoder is willing to tolerate from the remote peer. | ||
| private let maxFrameSize: Int | ||
| public let maxFrameSize: Int |
There was a problem hiding this comment.
we can also keep this internal if we like
There was a problem hiding this comment.
my vote would go to let's keep it internal :)
There was a problem hiding this comment.
Yup, I think we should keep it internal too.
There was a problem hiding this comment.
It's always easier to make things more public than to make them less public.
| public init(shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?, | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>) { | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>, maxFrameSize: Int = 1 << 14) { | ||
| precondition(maxFrameSize <= UInt32.max, "invalid overlarge max frame size") |
There was a problem hiding this comment.
if you're already preconditioning here, mind checking it's positive too?
There was a problem hiding this comment.
Is there any point in this precondition? I think it just duplicates logic that exists in the web socket frame decoder.
There was a problem hiding this comment.
@Lukasa imho its better to fail fast... WDYT ?
|
|
||
| /// The maximum frame size the decoder is willing to tolerate from the remote peer. | ||
| private let maxFrameSize: Int | ||
| public let maxFrameSize: Int |
There was a problem hiding this comment.
Yup, I think we should keep it internal too.
|
|
||
| /// The maximum frame size the decoder is willing to tolerate from the remote peer. | ||
| private let maxFrameSize: Int | ||
| public let maxFrameSize: Int |
There was a problem hiding this comment.
It's always easier to make things more public than to make them less public.
| public init(shouldUpgrade: @escaping (HTTPRequestHead) -> HTTPHeaders?, | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>) { | ||
| upgradePipelineHandler: @escaping (Channel, HTTPRequestHead) -> EventLoopFuture<Void>, maxFrameSize: Int = 1 << 14) { | ||
| precondition(maxFrameSize <= UInt32.max, "invalid overlarge max frame size") |
There was a problem hiding this comment.
Is there any point in this precondition? I think it just duplicates logic that exists in the web socket frame decoder.
| /// 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) { |
There was a problem hiding this comment.
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.
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Definitely meant request changes, sorry.
| self.shouldUpgrade = shouldUpgrade | ||
| self.upgradePipelineHandler = upgradePipelineHandler | ||
| self.maxFrameSize = maxFrameSize | ||
| } |
There was a problem hiding this comment.
Curious, what is the motivation for providing two public constructors rather than a constructor with the default value?
There was a problem hiding this comment.
There was a problem hiding this comment.
@vlm Short answer is "to preserve the trailing closure".
|
|
||
| /// The maximum frame size the decoder is willing to tolerate from the remote peer. | ||
| private let maxFrameSize: Int | ||
| /* private but tests */ let maxFrameSize: Int |
There was a problem hiding this comment.
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.
@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.
4 participants
Motivation:
At the moment its not possible to adjust the max websockets frame size when using the upgrader while its possible when directly use the WebSocketFrameDecoder.
Modifications:
Allow to specify max frame size via an init argument (with default value).
Result:
More flexible way to use the upgrader.