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

mmiller-max · 2020-10-06T11:05:03Z

This PR adds the functionality for the user to add one or more functions to be run when the server is shutting down. This is useful for things like ensuring database connections are closed.

Example use:

# One function
myshutdownfn() = println("I'm shutting down")

function run()
    HTTP.serve(requestHandler, "0.0.0.0", 5082, on_shutdown=myshutdownfn)
end

# Multiple functions
myshutdownfn1() = println("I'm shutting down")
myshutdownfn2() = println("I'm still shutting down")

function run()
    HTTP.serve(requestHandler, "0.0.0.0", 5082, on_shutdown=[myshutdownfn1, myshutdownfn2])
end

This needs some documentation/examples/tests, but wanted to check the approach was okay first.

Closes #591.

codecov-commenter · 2020-10-06T11:08:53Z

Codecov Report

Merging #599 into master will increase coverage by 0.02%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   77.86%   77.89%   +0.02%     
==========================================
  Files          37       37              
  Lines        2435     2438       +3     
==========================================
+ Hits         1896     1899       +3     
  Misses        539      539

Impacted Files	Coverage Δ
src/Servers.jl	`58.45% <60.00%> (-0.55%)`	⬇️
src/ConnectionPool.jl	`78.03% <0.00%> (+0.32%)`	⬆️
src/IOExtras.jl	`75.86% <0.00%> (+3.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34fccfa...94f8bea. Read the comment docs.

src/Servers.jl

fredrikekre · 2020-10-06T11:12:55Z

src/Servers.jl

 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))


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

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.

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.

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.

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 :)

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).

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

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!

src/Servers.jl

mmiller-max · 2020-10-07T11:44:37Z

Added try/catch to stop an erring shutdown function stopping the server from closing.

I've realised that the shutdown functions won't run if an IOServer object that has been supplied to listen is closed, as close(s::Server) will never be called. However if someone is manually closing their server then they can probably do anything they need to do before it.

test/server.jl

codecov-io · 2020-10-07T11:58:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@34fccfa). Click here to learn what that means.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #599   +/-   ##
=========================================
  Coverage          ?   77.43%           
=========================================
  Files             ?       37           
  Lines             ?     2437           
  Branches          ?        0           
=========================================
  Hits              ?     1887           
  Misses            ?      550           
  Partials          ?        0

Impacted Files	Coverage Δ
src/Servers.jl	`59.31% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34fccfa...103f1af. Read the comment docs.

codecov-commenter · 2020-10-10T12:02:59Z

Codecov Report

Merging #599 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   77.86%   77.95%   +0.09%     
==========================================
  Files          37       37              
  Lines        2435     2441       +6     
==========================================
+ Hits         1896     1903       +7     
+ Misses        539      538       -1

Impacted Files	Coverage Δ
src/Servers.jl	`60.68% <100.00%> (+1.69%)`	⬆️
src/ConnectionPool.jl	`78.03% <0.00%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34fccfa...165f338. Read the comment docs.

codecov-io · 2020-10-10T12:06:46Z

Codecov Report

Merging #599 into master will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   77.86%   78.20%   +0.34%     
==========================================
  Files          37       37              
  Lines        2435     2441       +6     
==========================================
+ Hits         1896     1909      +13     
+ Misses        539      532       -7

Impacted Files	Coverage Δ
src/Servers.jl	`66.66% <100.00%> (+7.67%)`	⬆️
src/debug.jl	`36.36% <0.00%> (-5.31%)`	⬇️
src/status_messages.jl	`98.78% <0.00%> (-1.22%)`	⬇️
src/URIs.jl	`92.71% <0.00%> (-0.67%)`	⬇️
src/sniff.jl	`86.76% <0.00%> (-0.10%)`	⬇️
src/ConnectionPool.jl	`77.63% <0.00%> (-0.08%)`	⬇️
src/IOExtras.jl	`75.86% <0.00%> (+3.44%)`	⬆️
src/RetryRequest.jl	`60.00% <0.00%> (+5.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34fccfa...0b2f07e. Read the comment docs.

mmiller-max · 2020-10-10T12:10:01Z

I've modified the tests to actually test close rather than shutdown itself. I'm not sure why the Labeler is failing - do I need to squash?

Apart from that I think this is ready.

mmiller-max · 2020-10-18T21:30:29Z

Squash didn't help with the labeler. Any help with this would be appreciated

mmiller-max · 2020-10-28T00:52:45Z

Just bumping.

quinnj

Sorry for the slow review; this looks good to me.

mmiller-max · 2020-11-16T21:31:56Z

@quinnj no worries, thanks for reviewing and merging!

fredrikekre reviewed Oct 6, 2020

View reviewed changes

mmiller-max commented Oct 7, 2020

View reviewed changes

test/server.jl Outdated Show resolved Hide resolved

Add option to run functions on server shutdown

0b2f07e

mmiller-max force-pushed the mm/onshutdown branch from 165f338 to 0b2f07e Compare October 18, 2020 21:29

quinnj approved these changes Nov 12, 2020

View reviewed changes

quinnj merged commit 54bb43b into JuliaWeb:master Nov 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mmiller-max commented Oct 6, 2020 •

edited

Loading

codecov-commenter commented Oct 6, 2020 •

edited

Loading

fredrikekre Oct 6, 2020

mmiller-max Oct 6, 2020

fredrikekre Oct 6, 2020

mmiller-max Oct 6, 2020

fredrikekre Oct 6, 2020

bauglir Nov 11, 2020

mmiller-max Nov 16, 2020

bauglir Nov 17, 2020

mmiller-max commented Oct 7, 2020

codecov-io commented Oct 7, 2020

codecov-commenter commented Oct 10, 2020

codecov-io commented Oct 10, 2020 •

edited

Loading

mmiller-max commented Oct 10, 2020

mmiller-max commented Oct 18, 2020

mmiller-max commented Oct 28, 2020

quinnj left a comment

mmiller-max commented Nov 16, 2020

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

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

Conversation

mmiller-max commented Oct 6, 2020 • edited Loading

codecov-commenter commented Oct 6, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmiller-max commented Oct 7, 2020

codecov-io commented Oct 7, 2020

Codecov Report

codecov-commenter commented Oct 10, 2020

Codecov Report

codecov-io commented Oct 10, 2020 • edited Loading

Codecov Report

mmiller-max commented Oct 10, 2020

mmiller-max commented Oct 18, 2020

mmiller-max commented Oct 28, 2020

quinnj left a comment

Choose a reason for hiding this comment

mmiller-max commented Nov 16, 2020

mmiller-max commented Oct 6, 2020 •

edited

Loading

codecov-commenter commented Oct 6, 2020 •

edited

Loading

codecov-io commented Oct 10, 2020 •

edited

Loading