I don’t like losing log messages if I ctrl-C my server, because they haven’t been flushed yet. And I don’t like that in-flight requests are immediately killed. Especially because this isn’t logged, so you can’t actually know how often this happens.
I wrote this:
It seems to work for me in about 2 minutes of testing. But I’m sure there’re lots of ways to improve it.
Any thoughts, ideas, or suggestions on how I could improve it?
Would we consider putting it into the standard library, perhaps even in the HTTP::Server implementation itself? (A parameter could be passed at initialization to enable the ‘wait’ before exiting the listen call, to ensure no changes to behavior in existing code.) Or is it better as a subclass?
The ‘done’ channel in ‘listen’ could be replaced with a wait group as well, is there any benefit in doing so?
I would publish this as a shard, if there’s no interest in adding it to the standard library.
In addition, I might investigate how to implement a timeout on incoming requests. However, since there’s no way to cancel a running fiber, it’d leave them running and just throw away the results. Perhaps I could add an optional parameter of a channel to watch for a cancellation message, and then in the few cases where it’s possible to make the code run under a channel select, it could raise an appropriate error. But in that case, it could also implement the timeout itself…
I have run into the same problem and this is a really interesting solution. I hadn’t thought to add it to HTTP::Server. What I do is add a middleware entry that tracks in-flight requests and provides a wait method that polls an Atomic(Int32).
require "http"
class InFlightRequestTracker
include HTTP::Handler
@requests = Atomic(Int32).new(0)
def call(context)
@requests.add 1
call_next context
ensure
@requests.sub 1
end
def wait
until @requests.get.zero?
sleep 100.milliseconds
end
end
end
class App
include HTTP::Handler
def call(context)
sleep 10.seconds
end
end
in_flight = InFlightRequestTracker.new
http = HTTP::Server.new([
in_flight,
App.new,
])
# Trap INT and TERM signals, closing the server down
Array(Signal){:int, :term}.each &.trap { http.close }
http.listen 8080
# Wait for all in-flight requests to complete before exiting
in_flight.wait
I don’t know why I didn’t think of that; it’s a much simpler solution than mine, since it doesn’t require patching or subclassing HTTP::Server in any way.
Maybe once the concurrency/multithreading changes are implemented and settling down, it might be worth it to revisit adding it to HTTP::Server itself? I would love to either see it be the default (that #close waits for outstanding requests) or at least an easy parameter to enable. I don’t like “this loses data out of the box, good luck” solutions, and I think it’ll be a little easier on adoption if we don’t make it so easy for people to shoot themselves in the foot.
The same for flushing file descriptors on exit, if at all possible; Crystal is the only language I’ve used that doesn’t do that, and it’s jarring and takes a lot of effort to work around. But that’s another battle for another day.
This is close to my ideal, tbh. I don’t know if close itself is the right thing to block on (it’s executed out of band in the signal handler), but what I do in my background-job framework is that start blocks until all jobs are finished after calling stop. The equivalent would be HTTP::Server#listen waiting after close is called until the requests are handled.
Otherwise, we’re on the same track.
Your timing is impeccable. We’ve been discussing exactly that topic this week on the repo.
