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

Close connection without response on status code 444 #1134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fredrikekre
Copy link
Member

This patch adds the ability for stream handlers to bail out on a request and tell the internal HTTP.jl code to simply close the connection and to ignore any remaining data to be read or written.

This is done by setting the response status code to the non-standard value 444, just like in nginx. If the status code is set to 444 when the request handler returns, the internal server code simply closes the connection. In particular this will skip calling closeread and closewrite, which both do some "consistency checks" that can't be avoided by using something like

HTTP.setstatus(http, 444)
close(http.stream)

inside the request handler function directly.

This patch adds the ability for stream handlers to bail out on a request
and tell the internal HTTP.jl code to simply close the connection and to
ignore any remaining data to be read or written.

This is done by setting the response status code to the non-standard
value 444, just like in nginx. If the status code is set to 444 when
the request handler returns, the internal server code simply closes the
connection. In particular this will skip calling `closeread` and
`closewrite`, which both do some "consistency checks" that can't be
avoided by using something like

```julia
HTTP.setstatus(http, 444)
close(http.stream)
```

inside the request handler function directly.
@quinnj
Copy link
Member

quinnj commented Dec 10, 2023

This seems fine to me; could you expound just a bit more on why it's beneficial to just close and ignore vs. the normal calling closeread/closewrite?

@fredrikekre
Copy link
Member Author

With this server

HTTP.listen(8123; access_log=combined_logfmt) do http
    if http.message.target == "/normal"
        HTTP.setstatus(http, 400)
        HTTP.startwrite(http)
    elseif http.message.target == "/444"
        HTTP.setstatus(http, 444)
    end
    return
end

we get with curl for example:

$ curl -d "{}" localhost:8123/normal
curl: (18) transfer closed with outstanding read data remaining

$ curl -d "{}" localhost:8123/444
curl: (52) Empty reply from server

but also on the Julia server logs there is

┌ Error: handle_connection handler error.
│
│ ===========================
│ HTTP Error message:
│
│ ERROR: EOFError: read end of file
│ Stacktrace:
│  [1] closeread
│    @ ~/dev/HTTP/src/Streams.jl:395 [inlined]
│  [2] handle_connection(f::Function, c::HTTP.Connections.Connection{Sockets.TCPSocket}, listener::HTTP.Servers.Listener{Nothing, Sockets.TCPServer}, readtimeout::Int64, access_log::Function)
│    @ HTTP.Servers ~/dev/HTTP/src/Servers.jl:460
│  [3] macro expansion
│    @ ~/dev/HTTP/src/Servers.jl:386 [inlined]
│  [4] (::HTTP.Servers.var"#16#17"{var"#17#18", HTTP.Servers.Listener{Nothing, Sockets.TCPServer}, Set{HTTP.Connections.Connection}, Int64, HTTP.var"#4#5", Base.Semaphore, HTTP.Connections.Connection{Sockets.TCPSocket}})()
│    @ HTTP.Servers ./task.jl:514
└ @ HTTP.Servers ~/dev/HTTP/src/Servers.jl:472

for the first request.

Like in nginx, the 444 code can now be used to just close and move on, without even bothering sending response headers, reading request data, etc.

@gustafsson
Copy link
Contributor

It seems convenient to be able to bail out completely, but is it only nginx that uses 444 like this? I can't seem to find any other references to this practice.

Since the status is code isn't even sent to the client why use a status code at all? Perhaps a custom exception "throw(BailOutError())" would make the intention more explicit?

Could the docs include some comment on a scenario for the intended usage of such functionality?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants