Fix things broken by @samoconnor in #135

[samoconnor]

• add server layer to enforce optional request size limits WIP: Factorisation of implementation into modules and layers #135 (comment) -- b2b5a14
  

  HTTP.jl/src/Messages.jl

  Line 456 in 54fd8f8

  if n > header_size_limit

• iso8859_1_to_utf8 in String(...) base on Content-Type. WIP: Factorisation of implementation into modules and layers #135 (comment) -- 6d978be 54fd8f8
  

  HTTP.jl/src/Messages.jl

  Line 400 in 54fd8f8

  payload(m::Message, ::Type{String}) =

• un-deprecate the headers and body keyword arguments Fix things broken by @samoconnor in #135 #141 (comment) 6bc739f 96fc3b6
• query= kw option to replace query part of url Fix things broken by @samoconnor in #135 #141 (comment) -- 57ff747
  

  HTTP.jl/src/HTTP.jl

  Line 298 in 57ff747

  uri = merge(uri, query=query)

• Transfer-Encoding decode Moved to Decode payload body based on message body Transfer-Encoding. #217
  

  HTTP.jl/src/Messages.jl

  Line 406 in 54fd8f8

  @warn "Decoding of HTTP Transfer-Encoding is not implemented yet!"

• sniff/gzip detection in Messages.bodysummary? WIP: Factorisation of implementation into modules and layers #135 (review)
  Messages.bodysummary only prints the first part of the body, so dumping a much of gziped binary isn't as bad as it was.
  

  HTTP.jl/src/Messages.jl

  Line 518 in b602c4e

  bodysummary(bytes) = view(bytes, 1:min(length(bytes), body_show_max))

• Reimplement the Content-Type detection code using sniff.jl WIP: Factorisation of implementation into modules and layers #135 (comment)
  

  HTTP.jl/src/ContentTypeRequest.jl

  Lines 20 to 21 in fdc264e

  	sn = sniff(bodybytes(body))
  	setkv(headers, "Content-Type", sn)

[EricForgy]

After a fresh Pkg.update() and restarting the REPL, it seems to hang...

After hitting Control-C, some more info...

Edit: Looks like it is getting stuck at addprocs(5) in runtests.jl.

[samoconnor]

hmm, looks like a problem with Distributed on your system?
Seems ok here: https://ci.appveyor.com/project/quinnj/http-jl/build/job/yjpcpif8prnk91v6

[EricForgy]

Could be. I can run addprocs(5) from the REPL with no problem though 🤔

[samoconnor]

This build has Win32 & Win64 passing for Julia v0.6: https://ci.appveyor.com/project/quinnj/http-jl/build/1.0.391

[quinnj]

One thing I'd like to add to the list is un-deprecating the headers and body keyword arguments. I feel like there are quite a few use-cases where you need to provide one or the other, but not both (or in the case of body, not headers). I feel like these are very natural keyword arguments that users reach for instead of having to always check the docs of HTTP.get or whatever to remember that it's a positional arg.

[samoconnor]

@quinnj any thoughts on 6d978be using load ?

[samoconnor]

un-deprecating the headers and body keyword arguments

Agree, I'll put this in my next commit.

[quinnj]

Hmmm, not sure I like load; I think most users just want it to convert automatically. Perhaps we can add a new keyword so it can be turned off if desired? Happy to take a stab at this if you want.

[samoconnor]

The question is "automatically" when?

Message.body is a Vector{UInt8} field as per the RFC message-body = *OCTET.

Other places where we used to have APIs like takebuf_string and readstring, have been deprecated in favour of String(take!(...) and read(..., String).

I.e. the base API for HTTP.Message.body most consistent with the HTTP RFC and with other similar Julia things seems to be just to access the Vector{UInt8} field.

An API that uses knowledge of the message headers to transform the body needs to take the HTTP.Message as an argument. Base.String(m::Message) is out because that rightly produces a string representation of the entire message.

What we seem to be looking for is a function that says "get me a string representation of this Response's body using metadata from the headers to interpret the body bytes".

We could have bodystring(::Message)::String. My hesitation with this is that it seems to go against the flow of the readstring -> read(..., String) deprecation.

We may also want to consider an API for the payload body (i.e. the message body with decoded Transfer-Encoding).

What about this:

payload(m::Message)::Vector{UInt8} = hasheader(m, "Transfer-Encoding") ?
                                     decode(header(m, "Transfer-Encoding"), m.body) :
                                     m.body
payload(m::Message, Type{String}) = hasheader(m, "Content-Type", "ISO-8859-1") ?
                                    return iso8859_1_to_utf8(payload(m)) :
                                    String(payload(m))
payload(m::Message, Type{T}) =
    ... various methods that figure out how to transform to type `T`
    based on Content-Type (and other headers?).
    This could include short-cut methods that know how to decode
    Transfer-Encoding and Content-Type in a single step.
    e.g. for gzip encoded JSON, or gzip encoded CSV.

[quinnj]

That all sounds reasonable to me. What if we just call it HTTP.body(resp) and HTTP.body(resp, String) instead?

[samoconnor]

Any place the RFC uses language or terminology with specific meanings we should be careful to follow as closely as possible to avoid confusion. In the RFC "message-body" is the raw form (possibly encoded); and "payload-body" is the pre-Transfer-Encoding form.
If we have a field called Message.body or a function called body(::Message) it should match what the RFC calls "message-body", anything else (i.e. having body::(Message) return the "payload body") is confusing. If we had a Payload type, then body(::Payload) -> "payload body" would be fine.
We could rename the payload function to payloadbody, to match the RFC terminology more precisely, I'd be happy with that, but on the other hand payload by itself doesn't seem to lead to any ambiguity and the RFC uses just "payload" to refer to the "payload body" in some places.

54fd8f8

[quinnj]

Alright, HTTP.payload seems fine then.

[samoconnor]

re kw args: 6bc739f 96fc3b6

[samoconnor]

See also re-introduction of Response(status, body) constructors: 2c14d43

[KristofferC]

As a HTTP noob, I thought the query keyword was kinda convenient but maybe there is a reason to remove it.

[samoconnor]

@KristofferC (& @quinnj) is the patch below what you're looking for with query= ?
i.e. the query= option passed to HTTP.request replaces the query part of the url argument.
No really important reason for the removal. I guess you could argue that the query is part of the url and that the user should do URI(scheme="http", host="foobar.com", path="/foo", query=["'a" => 1]) or merge(URI("http://foobar.com/foo"), query=["a" => 1]) or "http://foobar.com/foo?$(escapeuri(["a" => 1])". However, I can see that HTTP.request("GET", "http://foobar.com/foo"), query=["a" => 1]) is a nice shortcut.

--- a/src/HTTP.jl
+++ b/src/HTTP.jl
@@ -79,6 +79,10 @@ HTTP.put("http://httpbin.org/put", [], "Hello"; conf..)


+URL options
+
+ - `query = nothing`, replaces the query part of `url`.
+
 Streaming options

  - `response_stream = nothing`, a writeable `IO` stream or any `IO`-like
@@ -286,8 +290,15 @@ request(method::String, url::URI, headers::Headers, body; kw...)::Response =

 const nobody = UInt8[]

-request(method, url, h=Header[], b=nobody; headers=h, body=b, kw...)::Response =
-    request(string(method), URI(url), mkheaders(headers), body; kw...)
+function request(method, url, h=Header[], b=nobody;
+                 headers=h, body=b, query=nothing, kw...)::Response
+
+    uri = URI(url)
+    if query != nothing
+        uri = merge(uri, query=query)
+    end
+    return request(string(method), uri, mkheaders(headers), body; kw...)
+end

[EricForgy]

I completely uninstalled Julia and deleted all traces of v0.6, reinstalled Julia and my tests STILL won't run :(

But, tests run fine on my colleagues machines. Must be something with my machine. Argh.

Edit: @samoconnor I found the problem. The tests only run if I run the REPL as admin. Is that normal? Why should I need to be admin to run a test?

Edit^2: Before running the REPL as admin, this is what happens after the test freezes and I hit Ctrl-C:

The EXCEPTION_ACCESS_VIOLATION made me think to run the REPL as admin and then the test ran fine.

[samoconnor]

@amitmurthy ? Any guesses as to the cause of @EricForgy's problem using addprocs as a non-admin user on windows?

[EricForgy]

@amitmurthy ? Any guesses as to the cause of @EricForgy's problem using addprocs as a non-admin user on windows?

Just an additional note, I can addprocs(5) fine from the REPL without being admin, but when HTTP/tests/server.jl calls addprocs(5), it hangs unless I am admin.

[EricForgy]

Question:

HTTP.Servers module exports Servers, ServerOptions and serve. However, HTTP.jl just calls using Servers.listen. Should there be a using Servers so we can serve with HTTP.serve()?

[samoconnor]

Re listen vs serve, see JuliaWeb/GitHub.jl@b902a68
My hope is that serve can be deprecated and that listen is as convenient to use as serve was (while being more flexible by allowing ... do ::Stream).

[EricForgy]

My hope is that serve can be deprecated and that listen is as convenient to use as serve was (while being more flexible by allowing ... do ::Stream).

Ah. Ok. I will try to use listen instead 😊

Btw, I am going to try to switch https://github.com/EricForgy/Pages.jl from HttpServer.jl to HTTP.jl and my team is waiting on me, so that is one reason why I'm trying to help out 😅

[EricForgy]

What would a minimal example using HTTP.listen look like?

Edit: Found this 👍

HTTP.listen() do http::HTTP.Stream
    @show http.message
    @show header(http, "Content-Type")
    while !eof(http)
        println("body data: ", String(readavailable(http)))
    end
    setstatus(http, 404)
    setheader(http, "Foo-Header" => "bar")
    startwrite(http)
    write(http, "response body")
    write(http, "more response body")
end

HTTP.listen() do request::HTTP.Request
    @show header(request, "Content-Type")
    @show payload(request)
    return HTTP.Response(404)
end

[samoconnor]

From the doc:

HTTP.jl/src/Servers.jl

Lines 260 to 271 in 88f086a

	HTTP.listen() do http::HTTP.Stream
	@show http.message
	@show header(http, "Content-Type")
	while !eof(http)
	println("body data: ", String(readavailable(http)))
	end
	setstatus(http, 404)
	setheader(http, "Foo-Header" => "bar")
	startwrite(http)
	write(http, "response body")
	write(http, "more response body")
	end

HTTP.jl/src/Servers.jl

Lines 273 to 277 in 88f086a

	HTTP.listen() do request::HTTP.Request
	@show header(request, "Content-Type")
	@show payload(request)
	return HTTP.Response(404)
	end

Example in test/async.jl

HTTP.jl/test/async.jl

Lines 14 to 32 in 88f086a

	@async HTTP.listen() do http
	startwrite(http)
	write(http, """
	<html><head>
	<title>HTTP.jl Connection Pool</title>
	<meta http-equiv="refresh" content="1">
	<style type="text/css" media="screen">
	td { padding-left: 5px; padding-right: 5px }
	</style>
	</head>
	<body><pre>
	""")
	write(http, "<body><pre>")
	buf = IOBuffer()
	HTTP.ConnectionPool.showpoolhtml(buf)
	write(http, take!(buf))
	write(http, "</pre></body>")
	end

Example in GitHub.jl:
JuliaWeb/GitHub.jl@b902a68#diff-91d13e6f489f508c1d1e0f88f1e6bbb8R126

[EricForgy]

FYI: I've migrated the HTTP parts of Pages.jl from HttpServer.jl to HTTP.jl.

https://github.com/EricForgy/Pages.jl/tree/HTTP.jl

Now I need to get WebSockets working for the rest 😅

[samoconnor]

Hi @EricForgy,

Please try HTTP.Websockets.[listen()|open()]

See the source for function arguments:

HTTP.jl/src/WebSockets.jl

Line 114 in 6c10abc

function listen(f::Function,

HTTP.jl/src/WebSockets.jl

Line 80 in 6c10abc

function open(f::Function, url; binary=false, verbose=false, kw...)

Example of client in tests:

HTTP.jl/test/WebSockets.jl

Lines 7 to 22 in 6c10abc

	HTTP.WebSockets.open("$s://echo.websocket.org") do io
	write(io, Vector{UInt8}("Foo"))
	@test !eof(io)
	@test String(readavailable(io)) == "Foo"
	write(io, Vector{UInt8}("Hello"))
	write(io, " There")
	write(io, " World", "!")
	closewrite(io)
	buf = IOBuffer()
	write(buf, io)
	@test String(take!(buf)) == "Hello There World!"
	close(io)
	end

Example of client/server in discussion: JuliaWeb/WebSockets.jl#84 (comment)

Server:

using HTTP; HTTP.WebSockets.listen() do ws
           open(`vlc -q --play-and-exit --intf dummy -`, "w") do vlc_stdin
               while !eof(ws)
                   ogg_samples = readavailable(ws)
                   write(vlc_stdin, ogg_samples)
                   write(ws, "Streamed $(length(ogg_samples))-bytes...")
               end
           end
       end

Client:

julia> using HTTP; HTTP.WebSockets.open("ws://localhost:8081") do ws
           HTTP.open("GET", "http://tinyurl.com/bach-cello-suite-1-ogg"; verbose=2) do http
               while !eof(http)
                   write(ws, readavailable(http))
                   println(String(readavailable(ws)))
               end
           end
       end

[EricForgy]

Cool. Thank you 🙏 I will give it a try today (I'm in Bangkok).

One thing I liked about HttpServer / WebSockets was that the WS and HTTP were handled on the same port. Is that possible with HTTP.jl?

[EricForgy]

One thing I liked about HttpServer / WebSockets was that the WS and HTTP were handled on the same port. Is that possible with HTTP.jl?

I think I know how to do this now. Will give it a try 🙏

[samoconnor]

This issue has outlived its usefulness.
Pleas open new issues if something has been missed.
See #217 re transfer encoding.