Treat chunks as HTTP 1.1 #43
Conversation
| | 1, 1 -> "HTTP/1.1" | ||
| | _ -> "HTTP/1.0" | ||
| in | ||
| match reply_status with |
There was a problem hiding this comment.
| match reply_status with | |
| http_version ^ " " ^ match reply_status with |
or better yet split into extra function show_http_code
There was a problem hiding this comment.
I wanted to make the code clear that `Custom would override the http version. In one of my previous commits I assemble your commend c2e43d8 (#43)
Will try to came up with something more expressive
There was a problem hiding this comment.
Made it more apparent that `Custom is very custom 44c764e (#43)
httpev_common.ml
Outdated
| | `Not_implemented -> "HTTP/1.0 501 Not Implemented" | ||
| | `Service_unavailable -> "HTTP/1.0 503 Service Unavailable" | ||
| | `Version_not_supported -> "HTTP/1.0 505 HTTP Version Not Supported" | ||
| let show_http_reply : ?version:int * int -> reply_status -> string = |
There was a problem hiding this comment.
i would argue it should not be optional parameter, also
`Http_1_0 | `Http_1_1
httpev.ml
Outdated
| @@ -941,7 +941,8 @@ let send_reply c cout reply = | |||
| in | |||
| (* do not transfer body for HEAD requests *) | |||
| let body = match c.req with Ready { meth = `HEAD; _ } -> `Body "" | _ -> body in | |||
| let headers = make_request_headers code hdrs in | |||
| let version = match body with `Body _ -> Some (1, 0) | `Chunks _ -> Some (1, 1) in | |||
There was a problem hiding this comment.
so this is not clear from PR description - some clients would ignore chunks only because header says http/1.0 ?
There was a problem hiding this comment.
It interprets it wrong.
chunk encoding with 1.1 contains on each chunk, the chunk size as a "header", in case of sending chunks and http 1.0 the client interprets this as part of the chunk itself and in case of a browser it renders as HTML or uses as json.
httpev.ml
Outdated
| @@ -382,10 +382,10 @@ let set_blocking req = | |||
| req.blocking <- Some io; | |||
| io | |||
|
|
|||
| let make_request_headers code hdrs = | |||
| let make_request_headers ?version code hdrs = | |||
There was a problem hiding this comment.
here also should not be optional
There was a problem hiding this comment.
yeah, agree. but there's 2 cases that I'm not sure if this PR is miss-leading: send_reply_async and send_reply_blocking. I would keep 1.0 on both cases to avoid those kinds of things #43 (comment)
davesnx
force-pushed
the
support-http-11-header
branch
from
6da1873 to
44c764e
Compare
HTTP 1.1 adds
Transfer-Encoding: chunkedwhich is needed to stream HTMLhttps://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Transfer-Encoding#chunked
I updated only the case of
Chunksand didn't want to touch the sync (Body) since I suspect may cause issues. We could deploy staging with 1.1 enabled in the meantime.