Close active connections while stopping an HTTPServer

[pushkarnk]

If the HTTPServer is stopped while some connections are active, the connections
need to be closed in the stop() method. Currently, the stop() method only
closes the listening channel. The child channels, representing the active
connections, also need to be closed. The right way to do this is to allow the
ongoing transfers to complete and gracefully close these channels. To implement
this, we have borrowed the ServerQuiescingHelper code from NIOExtras. On a
server stop event, the server channel is closed. The open child channels that
represent active connections are then collected, they are allowed to serve
outstanding requests and then shutdown once they are all done. This also
ensures a graceful connection shutdown on the clients.

[weissi]

@pushkarnk are you using NIO's quiescing helper?

[weissi]

here's a super primitive HTTP server that will correctly quiesce: https://github.com/apple/swift-nio-extras/blob/master/Sources/HTTPServerWithQuiescingDemo/main.swift

When you tell it to stop (in the example that's triggered by Ctrl+C (SIGINT)), it will stop accepting fresh connections but it will continue to serve the outstanding requests and then shut down once they're all done.

The bulk of the work is done in ServerQuiescingHelper

[pushkarnk]

@weissi Thanks for the references. Quiescing seems like the right thing to do during an untimely HTTPServer.stop(). I'll take a look.

I guess Kitura-net does not quiesce. It does an abrupt closure of all the connections resulting in the sending of RST packets amidst data transfers.
cc @djones6

[weissi]

@pushkarnk you can implement abrupt closure too. Key is to collect all channels. You could copy the code for the quiescing handler and just close stuff instead of sending the quiescing event.

Would only need to replace this line : https://github.com/apple/swift-nio-extras/blob/master/Sources/NIOExtras/QuiescingHelper.swift#L114

With channel.close(promise: nil)

[pushkarnk]

@weissi Does closing an EventLoop not lead to the closing of all the Channels it is associated with?

[weissi]

@pushkarnk yes, it does, but generally you’re meant to able to share EventLoops. Shutting down EventLoops is a bit brutal as it kills the execution environment. If everybody implements shutdown by piggybacking on the EventLoop’s shutdown mechanism, bad things will happen. So it’s good style to be able to shut down without the EL going away.

[weissi]

Also, an EventLoop that’s shutting down is kinda dangerous, if you el.execute something, it’ll never be executed. That makes programming rather hard. Dispatch therefore opted to have one global pool that can never be shut down. That doesn’t quite work for servers but I dont think ELs should be shut down outside of tests/app shutdown.

[pushkarnk]

@pushkarnk yes, it does, but generally you’re meant to able to share EventLoops. Shutting down EventLoops is a bit brutal as it kills the execution environment. If everybody implements shutdown by piggybacking on the EventLoop’s shutdown mechanism, bad things will happen. So it’s good style to be able to shut down without the EL going away.

I get your point. Thanks!

[weissi]

Also eventually you’ll want to quiesce anyway, so why not implement something that’s pretty much quiescing already and when you’re ready you can switch to quiescing at any point in time. Do you need 100% bug for bug compat with kitura-net?

[pushkarnk]

Also eventually you’ll want to quiesce anyway, so why not implement something that’s pretty much quiescing already and when you’re ready you can switch to quiescing at any point in time. Do you need 100% bug for bug compat with kitura-net?

In this case, we'd prefer to do a graceful shutdown in Kitura-net too, because that seems like the right thing to do.

[weissi]

Also eventually you’ll want to quiesce anyway, so why not implement something that’s pretty much quiescing already and when you’re ready you can switch to quiescing at any point in time. Do you need 100% bug for bug compat with kitura-net?

In this case, we'd prefer to do a graceful shutdown in Kitura-net too, because that seems like the right thing to do.

I think that's the right call.

[pushkarnk]

Fixes #216

[pushkarnk]

Hi @weissi

As suggested ServerQuiescingHelper code from swift-nio-extras is great and serving the purpose here! We'd be happy to have it made available through NIO. For now I've copied this code to Sources/KituraNet/HTTP/NIOQuiescingHelper.swift where I've put a comment referencing the original repository. Please let me know if this is okay by you.

cc @ianpartridge

[weissi]

hey @pushkarnk is the code from NIO modified at all?

[pushkarnk]

Oh, no, I could actually add swift-nio-extras as a dependency