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

body streaming doesn't stream #2393

Closed
weissi opened this issue Jun 10, 2020 · 1 comment · Fixed by #2404
Closed

body streaming doesn't stream #2393

weissi opened this issue Jun 10, 2020 · 1 comment · Fixed by #2404
Labels
bug Something isn't working
Projects

Comments

@weissi
Copy link
Contributor

weissi commented Jun 10, 2020

Because of #2392, I've added the r.headers.remove(name: "content-length") to this echo server below:

import Vapor

var env = try Environment.detect()
try LoggingSystem.bootstrap(from: &env)

let app = Application(env)
defer { app.shutdown() }

app.on(.POST, "echo")  { request -> Response in
    let r = Response(body: .init(stream: { writer in
        request.body.drain { body in
            switch body {
            case .buffer(let buffer):
                return writer.write(.buffer(buffer))
            case .error(let error):
                return writer.write(.error(error))
            case .end:
                return writer.write(.end)
            }
        }
    }, count: 0))
    r.headers.remove(name: "content-length")
    return r
}

try app.run()

I would expect this to stream. To check that it does, I wrote this shell script:

( echo -ne 'POST /echo HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n3\r\nFOO\r\n'; sleep 1; echo -ne '3\r\nBAR\r\n0\r\n\r\n'; sleep 1; ) | nc localhost 8080

which writes the request head + the first three bytes (FOO), then waits a second until it writes the next three bytes (BAR). Sure, HTTP chunks aren't semantic but 1 second should really be long enough for them to be sent back individually (and not concatenated).

They're sent back concatenated which means that Vapor must buffer them in memory!?

Expected:

HTTP/1.1 200 OK
connection: keep-alive
date: Wed, 10 Jun 2020 22:40:08 GMT
transfer-encoding: chunked

3\r\n
FOO\r\n
3\r\n
BAR\r\n
0

Actual:

HTTP/1.1 200 OK
connection: keep-alive
date: Wed, 10 Jun 2020 22:40:08 GMT
transfer-encoding: chunked

6
FOOBAR
0

I know that this is totally legal for Vapor to do but given that I waited a whole second, I would have really expected for those bytes to come back in two chunks.

For a .pcap run

sudo tcpdump -i lo0 -w /tmp/test.pcap '' 'port 8080' & sleep 1;  ( echo -ne 'POST /echo HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n3\r\nFOO\r\n'; sleep 1; echo -ne '3\r\nBAR\r\n0\r\n\r\n'; sleep 1; ) | nc localhost 8080; sudo pkill -INT tcpdump

Another way of showing this would be

( echo -ne 'POST /echo HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n3\r\nFOO\r\n'; sleep 100; ) | nc localhost 8080

where I only write the first chunk, and then wait 100 seconds. Sure, the "client" never completes the request but the FOO should've been replied back.

@tanner0101 tanner0101 added the bug Something isn't working label Jun 22, 2020
@tanner0101 tanner0101 added this to To Do in Vapor 4 via automation Jun 22, 2020
@tanner0101
Copy link
Member

Vapor collects request bodies by default. To stream requests, use the body parameter as described here: https://docs.vapor.codes/4.0/routing/#body-streaming

- app.on(.POST, "echo")  { request -> Response in
+ app.on(.POST, "echo", body: .stream) { request -> Response in

However, even with body streaming enabled, Vapor doesn't currently allow for a response to be sent before the current request has been entirely consumed. I've started a PR to tackle this issue here: #2404

Vapor 4 automation moved this from To Do to Done Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Vapor 4
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants