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

max body size #205

Closed
tanner0101 opened this Issue Jan 22, 2018 · 0 comments

Comments

Projects
None yet
1 participant
@tanner0101
Copy link
Member

tanner0101 commented Jan 22, 2018

Introduce some configuring for streaming on the HTTPParser (that will be settable via HTTPServer and finally EngineServer) that will enable sync decode APIs.

Details

The follow methods will be introduced on ContentContainer:

// will throw an error if HTTP body is of streaming type
let user = try req.content.decode(User.self) // User

// will work for all HTTP bodies
let user = try req.content.streamingDecode(User.self) // Future<User>

This will allow us to create a sync API for req.content.decode which a lot of users are requesting.

Methods

Now the hard part, how do we let the user decide which bodies should be streaming?

Streaming Threshold

One possibility is a "streaming threshold"

if contentLength > streamingThreshold  || contentLength == nil /* chunked */ {
    // configure body stream
    body = .stream(...)
} else {
    // collect data into buffer
    body = .data(...)
}

Assuming a threshold of around 1MB, the large majority of HTTP requests will fall under the streaming threshold and fit into a .data body.

Router Config

A method for passing in special config for routes could be used to specify routes that should accept streaming vs. non-streaming bodies:

router.post("users", streaming: true) { req in
    // req.body will always be of type streaming
}

Or even a subgroup:

router.streaming.post("users") {... }

Middleware

Another idea was to use a middleware to collect streaming bodies into Data. This middleware could then be optionally applied to routes or the application as a whole.

router.grouped(StreamCollector()).post("users") { req in
    // req.body will always be collected into Data
}

Middleware + Responder

A problem with the Middleware idea is that Stream collection should be the default with opt-out behavior ideally.

We could potentially get around this behavior by inserting the "Stream Collecting" behavior into the App's default responder. A middleware could then be created that would later flag the responder to not collect streamed bodies. Thus, if no middleware is present, body streams will be collected by default.

router.grouped(HTTPStreamingBodyEnabler()).post("users") { req in

}

With convenience:

extension Router {
    public var streaming: RouteGroup { return grouped(HTTPStreamingBodyEnabler()) }
}

router.streaming.post("users") { req in ... }

Global flag

Or, perhaps more simply, just a global flag: streamBodies: Bool. If false, all bodies w/ content length would be collected into Data first.

@tanner0101 tanner0101 added this to the 3.0.0 milestone Jan 22, 2018

@tanner0101 tanner0101 self-assigned this Jan 22, 2018

@tanner0101 tanner0101 closed this Feb 10, 2018

@tanner0101 tanner0101 added this to Done in Vapor 3 Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment