Consume body on res.respond if user does not call req.body

[kevinkassimo]

Fix #40

Consume body on req.respond() if the user fails to call req.body()

[bartlomieju]

Should we allow to consume body multiple times?

[kevinkassimo]

Do you mean that you expect req.body() to return the content even after respond, or do you mean that invoking multiple times of req.body() would all yield empty string here?

[bartlomieju]

IMHO error should be thrown if one tries to consume body after respond - it'll be less surprising for users and would allow to spot mistakes in code.

[kevinkassimo]

Sure. Updated.

[ry]

@kevinkassimo I'm a little concerned about this patch. I'm imaging an HTTP/1.0 connection (i.e. without keep-alive) in which the requester is uploading a file. Imagine the server gets the request, processes it and decides that it will not accept the upload, it wants to send back an html page with status 401 (Unauthorized). The server should simply stop reading from the socket - send the response - and then close the connection. If the server is forced to read a potentially very large upload every time it wants to reply, then this could be a DoS attack vector.

[bartlomieju]

I guess it becomes related to #22 as well, probably we should check how Go's server handles that case and follow similar pattern. OTOH if request has Connection: Keep-Alive and user doesn't consume body before responding we should consume it or error from #40 emerges.

[ry]

Since our server is a close port of Go's, I think it would be good if we researched how Go handles this situation. Does anyone have time to dive into https://github.com/golang/go/tree/master/src/net/http and pull up some links?

[bartlomieju]

I already read through some of it for #22, so I can handle that

[bartlomieju]

I went digging through Golang's codebase. Here some sythesis with links.

https://github.com/golang/go/blob/9e277f7d554455e16ba3762541c53e9bfc1d8188/src/net/http/server.go#L1818

// after establishing connection

w, err := c.readRequest(ctx)  // 1.
...
if err != nil {
    ...
}

// Expect 100 Continue support
...
serverHandler{c.server}.ServeHTTP(w, w.req)  // 2.
...
w.finishRequest()  // 3.
if !w.shouldReuseConnection() {  // 4.
    if w.requestBodyLimitHit || w.closedRequestBodyEarly() {
        c.closeWriteAndWait()
    }
    return
}
...

1. Read request from connection:
   1. parse first line
   2. headers
   3. quirk with host
   4. establish req.Close result of shouldClose
   5. set up body
   6. some check I do not understand, that actually controls what happens with body
2. Call user provided handler
3. Finish request
   1. ensure headers, flush writers, etc.
   2. close the body - this function checks if body was already consumed and if not it decides whether to consume or discard based on result from 1.6
4. Check if connection should be reused
   • basically if there was something to read in 3.2 and body was closed connection will not be reused

I have to admit that this logic is very complicated and I'm not experienced with Golang so please could someone double-check it?

[ry]

Thank you!

closedRequestBodyEarly and closeWriteAndWait is the smart idea I think we need here.

[kevinkassimo]

@bartlomieju I'll also look into details tonight. Thanks for diving in!

[kevinkassimo]

Actually, I'm planning to make HTTP more Go-ish in the next few days. It would probably be nice to just directly replicating the structure instead of developing some unreliable design ourselves...

[kevinkassimo]

Also, let's complete #76 first. Closing this.

I'll work on a best effort Go port after completing reorg

[ry]

I went digging through Golang's codebase. Here some sythesis with links.

closedRequestBodyEarly and closeWriteAndWait is the smart idea I think we need here.

@kevinkassimo @bartlomieju any updates on this?

[kevinkassimo]

@ry Quite busy these days and probably won't involve very much in the coming month. Hope someone else could take on the task.

[bartlomieju]

I'll look into it this week