vapor / http

🚀 Non-blocking, event-driven HTTP built on Swift NIO.
MIT License
240 stars 65 forks source link

syncShutdownGracefully() does not wait for all connections? #363

Closed ghost closed 5 years ago

ghost commented 5 years ago

I'm probably missing something obvious: Just wondering if it's possible to run an async operation off the main event loop in such a way that syncShutdownGracefully() or some other mechanism will safely wait for it to complete/finish before the server process ends? Right now it only seems to wait for the things are are actually running on the event loop. It would be nice if it waited for all open connections to return a response before the server process ends? I would like to shutdown the server and ensure that it does not drop any client connections. Is there a way to accomplish that currently? Thanks!

class Responder: HTTPServerDelegate {
    func respond(to req: HTTPRequest, on channel: Channel) -> EventLoopFuture<HTTPResponse> {
        let p = channel.eventLoop.makePromise(of: HTTPResponse.self)

    // if the server is closed/shutdown while getDataFrom3rdPartyService() is running on separate queue, so as not to block the event loop thread, the client connection will be dropped !!??
        DispatchQueue.global().async {
            let res = getDataFrom3rdPartyService()
            p.succeed(res)
        }

        return p.futureResult
    }
}
weissi commented 5 years ago

this NIO example implements what's asked here: https://github.com/apple/swift-nio-extras/blob/master/Sources/HTTPServerWithQuiescingDemo/main.swift . It uses ServerQuiescingHelper from swift-nio-extras to do so.

ghost commented 5 years ago

Thank you!

weissi commented 5 years ago

@jcl-dev does Vapor's http already use this technique? The above link is only an example from the NIO examples, not sure if Vapor uses it. Just wanted to point out that this is not too hard with NIO.

ghost commented 5 years ago

I appreciate it.

The vapor/http HTTPServer class appears to use ServerQuiescingHelper, but it does not appear to work in my testing if the request is processing off the channel.eventLoop. When I send a SIGTERM to my server process, it intercepts it and calls shutdown() on the HTTPServer instance. I'm probably just doing it wrong.

let group = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount)
let server = HTTPServer(
    configuration: .init(
        hostname: "0.0.0.0",
        port: port,
        maxBodySize: 10_000_000,
        supportVersions: .init([.one])),
    on: group)
_ = try! server.start(delegate: Responder()).wait()

// Allow the process to intercept the default kill signal which is SIGTERM in order to exit gracefully
signal(SIGTERM, SIG_IGN)
let sigterm = DispatchSource.makeSignalSource(signal: SIGTERM)
sigterm.setEventHandler {
    log.info("received SIGTERM")
    _ = server.shutdown()
}
sigterm.resume()

// Wait for the server to close (indefinitely).
try server.onClose.wait()

// Make sure to shutdown the group gracefully when the process exits.
try? group.syncShutdownGracefully()
log.info("graceful shutdown complete")
weissi commented 5 years ago

oh yes, it does indeed. If it doesn't work for you, then there must be some bug in there.

Can you confirm the NIO example does work for you?

ghost commented 5 years ago

Yes, I can confirm the NIO example you gave works as-is, and also works when modified to use DispatchQueue.global().asyncAfter to fulfill a promise:

let p = context.channel.eventLoop.makePromise(of: String.self)
DispatchQueue.global().asyncAfter(deadline: .now() + 10) {
    p.succeed("Fulfill!")
}
p.futureResult.whenSuccess { succeedString in
    buffer.clear()
    buffer.writeStaticString("done with the request now\n")
    buffer.writeString("\(succeedString)\n")
    context.write(self.wrapOutboundOut(.body(.byteBuffer(buffer))), promise: nil)
    context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
}
weissi commented 5 years ago

thanks for confirming this!

tanner0101 commented 5 years ago

The latest HTTP server stuff is here now: https://github.com/vapor/vapor/blob/master/Sources/Vapor/Server/HTTPServer.swift

Last I tested this (using telnet), server quiesce + sigterm was working as expected. There should really be a unit test for this, though.

ghost commented 5 years ago

Ah, thanks!