Allow WebSocket maxFrameSize to be configured

[bridger]

The maximum frame size of an incoming WebSocket packet can be configured. This fixes issues for apps that were running into the limit (#2195, fixes #2194).

let maxFrameSize: Int = 1024 * 1024 * 2 // 2mb
app.webSocket("api", "ws", maxFrameSize: .override(maxFrameSize)) { (request, ws) in
    websocketController.connected(request: request, connection: ws)
}

[Joannis]

@bridger Thanks for creating the issue, and a huge thanks for creating a PR to back it up. Really cool 👍

I think the API design could be more future-proof if we introduce a small change elsewhere.

[bridger]

I like the idea of returning a route that can be configured. I’d be happy to make that change if that’s preferred. I’m new to Vapor so I wasn’t sure about existing practices to follow.

_{Sent with GitHawk}

[Joannis]

We're in beta stages, so for now these changes are in a good spot for consideration.

[tanner0101]

+1 to adding this as a parameter. I do think it would be nice to be able to change the default maxFrameSize for all web socket connections as well, but that could be a separate PR. Something like:

app.webSocket.maxFrameSize = 1<<20

[bridger]

+1 to adding this as a parameter. I do think it would be nice to be able to change the default maxFrameSize for all web socket connections as well, but that could be a separate PR.

To support that case, I changed the parameter to be optional. Now the default (1<<14) is an internal detail. When we add an Application-level default, like app.webSocket.maxFrameSize then nil can indicate "use the application default".

[MrLotU]

LGTM. @tanner0101 could you give this another pass too? 😄

[tanner0101]

As a user, passing app.webSocket(maxFrameSize: nil) { ... } might make me think I'm disabling max frame size here. Maybe an enum instead would better express this?

enum WebSocketMaxFrameSize {
    case default
    case override(Int)
}

extension WebSocketMaxFrameSize: ExpressibleByIntegerLiteral {
    init(integerValue: Int) { 
        self = .override(integerValue)
    }
}

We could re-use this enum on the top-level config as well.

This may be overly pedantic though and perhaps a doc block explaining the behavior would be sufficient. What are your thoughts?

[bridger]

The enum is a good idea! I just updated the PR to use the enum.

[lmcd]

Can this be merged?

[bridger]

Can this be merged?

Apologies. There was an update to the PR that was waiting for me. I just made the change so I think it is good to merge.

[tanner0101]

Looks great, thanks!

[tanner0101]

These changes are now available in 4.0.0-rc.3.4