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

RFC: Enable user to run functions on server shutdown #599

Merged
merged 1 commit into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/Servers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,29 @@ struct Server{S <: Union{SSLConfig, Nothing}, I <: Base.IOServer}
server::I
hostname::String
hostport::String
on_shutdown::Any
end

Base.isopen(s::Server) = isopen(s.server)
Base.close(s::Server) = close(s.server)
Base.close(s::Server) = (shutdown(s.on_shutdown); close(s.server))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to pass s to the shutdown functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a possibility we might want to do other (not specified by the user) shutdown tasks that needed the other fields this would make sense? If not I think it's cleaner as it is for now, and it would always be easy to add it in the future with another method for shutdown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was just a thought that maybe you need the server object for shutting down your own stuff too, but yea can always be added later by checking applicable I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah applicable would let you do that, good idea. It's probably worth having the extra functionality in? I'll add it along with some tests and documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a usecase for it from top of my head, so maybe it can wait. Let's see what others have to say :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the past, I have built similar functionality in a wrapper around Server in a custom framework. I'm not sure if this use case would still be applicable here, but I used the shutdown functionality there to correctly close any open WebSockets instead of just destroying them (which can leave browsers confused).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that sounds like a good use case for this - are WebSockets destroyed when close(s.server) is performed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just prior to it. The stop function for the wrapper, which keeps track of open WebSockets, simply iterates over all the WebSockets that are still open, initiates closing them, waits for them to actually close and then calls close(wrapper.server).

Quite happy to see these types of changes making it into HTTP.jl. The main reason for this wrapper to exist, is for purposes such as this (and #602 which has also been fixed). Thanks for putting in the work!


"""
shutdown(fns::Vector{<:Function})
shutdown(fn::Function)
shutdown(::Nothing)

Runs function(s) in `on_shutdown` field of `Server` when
`Server` is closed.
"""
shutdown(fns::Vector{<:Function}) = foreach(shutdown, fns)
shutdown(::Nothing) = nothing
function shutdown(fn::Function)
try
fn()
catch e
@error "shutdown function $fn failed" exception=(e, catch_backtrace())
end
end

Sockets.accept(s::Server{Nothing}) = accept(s.server)::TCPSocket
Sockets.accept(s::Server{SSLConfig}) = getsslcontext(accept(s.server), s.ssl)
Expand Down Expand Up @@ -118,6 +137,10 @@ Optional keyword arguments:
allowed per client IP address; excess connections are immediately closed.
e.g. 5//1.
- `verbose::Bool=false`, log connection information to `stdout`.
- `on_shutdown::Union{Function, Vector{<:Function}, Nothing}=nothing`, one or
more functions to be run if the server is closed (for example by an
`InterruptException`). Note, shutdown function(s) will not run if a
`IOServer` object is supplied and closed by `close(server)`.

e.g.
```julia
Expand Down Expand Up @@ -203,7 +226,8 @@ function listen(f,
rate_limit::Union{Rational{Int}, Nothing}=nothing,
reuse_limit::Int=nolimit,
readtimeout::Int=0,
verbose::Bool=false)
verbose::Bool=false,
on_shutdown::Union{Function, Vector{<:Function}, Nothing}=nothing)

inet = getinet(host, port)
if server !== nothing
Expand All @@ -230,7 +254,7 @@ function listen(f,
x -> f(x) && check_rate_limit(x, rate_limit)
end

s = Server(sslconfig, tcpserver, string(host), string(port))
s = Server(sslconfig, tcpserver, string(host), string(port), on_shutdown)
return listenloop(f, s, tcpisvalid, connection_count,
reuse_limit, readtimeout, verbose)
end
Expand Down
24 changes: 24 additions & 0 deletions test/server.jl
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,28 @@ end
@test_skip HTTP.hasheader(HTTP.get("http://httpbin.org/redirect-to?url=https://httpbin.org/response-headers?Authorization=auth"), "Authorization")
end # @testset

@testset "on_shutdown" begin
@test HTTP.Servers.shutdown(nothing) === nothing

IOserver = Sockets.listen(Sockets.InetAddr(parse(IPAddr, "127.0.0.1"), 8052))

# Shutdown adds 1
TEST_COUNT = Ref(0)
shutdown_add() = TEST_COUNT[] += 1
server = HTTP.Servers.Server(nothing, IOserver, "host", "port", shutdown_add)
close(server)

# Shutdown adds 1, performed twice
@test TEST_COUNT[] == 1
server = HTTP.Servers.Server(nothing, IOserver, "host", "port", [shutdown_add, shutdown_add])
close(server)
@test TEST_COUNT[] == 3

# First shutdown function errors, second adds 1
shutdown_throw() = throw(ErrorException("Broken"))
server = HTTP.Servers.Server(nothing, IOserver, "host", "port", [shutdown_throw, shutdown_add])
@test_logs (:error, r"shutdown function .* failed") close(server)
@test TEST_COUNT[] == 4
end # @testset

end # module