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

Provide Coordinated Shutdown for Akka HTTP #1210

Open
jlprat opened this issue Jun 19, 2017 · 23 comments
Open

Provide Coordinated Shutdown for Akka HTTP #1210

jlprat opened this issue Jun 19, 2017 · 23 comments

Comments

@jlprat
Copy link
Member

@jlprat jlprat commented Jun 19, 2017

Make sure coordinated shutdown can be used in Akka HTTP, so pending requests can be processed before shutting the server down.

This idea originated as a comment from @raboof in #1207

Would be cool if we had a nice way to "drain" ("stop listening for new requests but finish in-flight ones and shut down", with a timeout). Can we do that?

Further info: http://doc.akka.io/docs/akka/current/scala/actors.html#coordinated-shutdown
Gitter Message

@ktoso

This comment has been minimized.

Copy link
Member

@ktoso ktoso commented Jun 19, 2017

I just checked, it seems that indeed we never got to adding http to the phases of coordinated shutdown. So here we need to actually do that, and then document using coordinated shutdown with akka http :)

Somewhat related #188

@jlprat

This comment has been minimized.

Copy link
Member Author

@jlprat jlprat commented Jun 19, 2017

Updating the ticket then.

@jlprat jlprat changed the title Check if Coordinated Shutdown is usable for Akka HTTP Provide Coordinated Shutdown for Akka HTTP Jun 19, 2017
@idzivinskyi

This comment has been minimized.

Copy link

@idzivinskyi idzivinskyi commented Oct 23, 2017

@jlprat @ktoso What the status of this task? Is there a workaround for this solution?

As I understand from a bunch of opened and closed tickets serverBinding.unbind will just reject new connections when the opened connections still will work. In that case, does this code make any sense?

  private val bind: Future[ServerBinding] = Http().bindAndHandle(flow, host, port)

  CoordinatedShutdown(system).addTask(
    CoordinatedShutdown.PhaseBeforeServiceUnbind, "http_shutdown") { () =>

    bind.flatMap(_.unbind).flatMap { _ =>
      Http().shutdownAllConnectionPools
    }.map { _ =>
      Done
    }
  }
@ktoso

This comment has been minimized.

Copy link
Member

@ktoso ktoso commented Oct 23, 2017

The status is as you see ;-) There's no hidden magical communication channels.

It's the wrong phase I think, rather this should be in:

  /**
   * Stop accepting new incoming requests in for example HTTP.
   */
  val PhaseServiceUnbind = "service-unbind"

as documented. Would you like to take a stab at this issue and submit a PR?

@idzivinskyi

This comment has been minimized.

Copy link

@idzivinskyi idzivinskyi commented Oct 25, 2017

@ktoso I would like to do it, but I have a question. unbind will just stop all new connections. How to stop all incoming connections? Http().shutdownAllConnectionPools - it looks like it does not make any sense without closing all incoming connections.

@2m 2m added 2 - pick next and removed 1 - triaged labels Nov 21, 2017
@jonas

This comment has been minimized.

Copy link
Member

@jonas jonas commented Dec 9, 2017

Also related with #907

@jrudolph

This comment has been minimized.

Copy link
Member

@jrudolph jrudolph commented Dec 11, 2017

Http().shutdownAllConnectionPools will forcibly stop all outgoing connections. unbind will stop accepting new incoming connections but it doesn't kill existing incoming ones, that would have to be implemented as well.

@johanandren

This comment has been minimized.

Copy link
Member

@johanandren johanandren commented Jan 30, 2018

Note that it should be possible to opt out of this, since specific applications may have other needs (for example keep the HTTP server alive and respond after leaving the cluster)

@wsargent

This comment has been minimized.

Copy link
Contributor

@wsargent wsargent commented Feb 27, 2018

Also related: #59

@ktoso ktoso added the t:core label May 28, 2018
@ktoso ktoso modified the milestones: 10.0.x, 10.1.x May 28, 2018
@ignasi35

This comment has been minimized.

Copy link
Member

@ignasi35 ignasi35 commented Jul 17, 2018

Note that it should be possible to opt out of this, since specific applications may have other needs (for example keep the HTTP server alive and respond after leaving the cluster)

+1 with a preference for opt-in vs opt-out. I think Akka HTTP as a library should not register any task on CoordinatedShutdown by default and instead provide docs or code to achieve it.

@ignasi35

This comment has been minimized.

Copy link
Member

@ignasi35 ignasi35 commented Jul 17, 2018

Really late to the party,. just noticed #2072 😅

@ktoso

This comment has been minimized.

Copy link
Member

@ktoso ktoso commented Jul 17, 2018

Really late to the party,. just noticed #2072 😅

Right though that's different @ignasi35 -- that's something you can opt in and call it. And currently there is even a large "TODO" in the docs about this ticket: https://github.com/akka/akka-http/pull/2072/files#diff-3f0330cd4e8b60684df0fcf735b85c78R67

That is "termination" which is a feature that you can trigger, and in this ticket we're talking about more docs or providing default Coordinated Shutdown. Even if it is about deciding that we don't enable by default.

Could you explain in more depth why you think in a pure akka-http app it should not register this?
A Play app could disable the hook enabled by akka perhaps by a config override etc is what I'm thinking if you think those would conflict for some reason?

@ignasi35

This comment has been minimized.

Copy link
Member

@ignasi35 ignasi35 commented Jul 18, 2018

If Akka-HTTP registers tasks into Coordinated Shutdown it will need to provide a means to disable them using a conf setting or something (as mentioned above). I find this default to be counter-intuitive.

I think Akka should provide the tools and the docs recommending how to use them.

Play and Lagom, OTOH, try to hide all the machinery and provide default setups that are reasonable enough. I that case I do think registering tasks by default is the right approach.


Right though that's different

Oh, I see the difference now. 🤦‍♂️

Then, I think this issue should only document how to connect the graceful termination with Coordinated Shutdown and maybe include some recommendations on when and when not to do it?

IIUC, at the moment it's possible to close akka HTTP using unbind or terminate so not adding default tasks still leaves it to a user to choose what to invoke.

I'm still not 100% decided what the best option is but, ATM, I think only docs and not register Coordinated Shutdown tasks is the right choice. Could be convinced otherwise. 😄

@michaelpisula

This comment has been minimized.

Copy link

@michaelpisula michaelpisula commented Aug 31, 2018

If only docs and not register Coordinated Shutdown tasks is the agreed solution, I could take a stab at adding it to the docs, I have a working solution I posted some weeks back on the discuss thingy. Would be happy to help :-)

@hseeberger

This comment has been minimized.

Copy link
Contributor

@hseeberger hseeberger commented Mar 18, 2019

I think in order to properly support Coordinated Shutdown we need to modify or supplement terminate, because unbind should happen during PhaseServiceUnbind while the other stuff (finishing already accepted requests, draining new requests on established connections, etc.) should happen during PhaseServiceRequestsDone.

Would it be possible to expose the terminateAction or offer a method which delegates to that?

Maybe we should even rename terminate to unbindAndTermninate and use the name terminate for a method which only does what the current terminateAction does.

@jroper

This comment has been minimized.

Copy link
Contributor

@jroper jroper commented Apr 18, 2019

@hseeberger unbind() is idempotent, I had a look at the code, all it does is delegate to the Akka IO ServerBinding.unbind, which eventually delegates to this method, which is clearly idempotent:

https://github.com/akka/akka/blob/87354a6c9e61cdaacbaef8ddd116f7d56d5b47ea/akka-stream/src/main/scala/akka/stream/impl/io/TcpStages.scala#L142-L148

So it doesn't really matter if terminate calls it - the only negative side effect may be that an Unbind message gets dead lettered - this can probably be avoided by checking if the unbindPromise is completed before sending the Unbind message here.

So this is the way I'm doing coordinated shutdown:

      val shutdown = CoordinatedShutdown(system)

      shutdown.addTask(CoordinatedShutdown.PhaseServiceUnbind, "http-unbind") { () =>
        binding.unbind().map(_ => Done)
      }

      shutdown.addTask(CoordinatedShutdown.PhaseServiceRequestsDone, "http-graceful-terminate") { () =>
        binding.terminate(10.seconds).map(_ => Done)
      }

      shutdown.addTask(CoordinatedShutdown.PhaseServiceStop, "http-shutdown") { () =>
        Http().shutdownAllConnectionPools().map(_ => Done)
      }

Not sure how necessary the last task is - they'll be cleaned up anyway when the actor system terminates.

@hseeberger

This comment has been minimized.

Copy link
Contributor

@hseeberger hseeberger commented Apr 18, 2019

@jroper thanks. I had already figured out that unbind is idempotent and I'm shutting down exactly like you have shown ;-)

@cyburgee

This comment has been minimized.

Copy link

@cyburgee cyburgee commented Apr 30, 2019

@jroper I've implemented a graceful shutdown similar to how you've demonstrated, however I'm seeing that chunked responses are being munged and return garbage even if the service is just returning a small bit of json that definitely could be returned within the termination hard deadline. have you seen or dealt with this issue?

@mikela

This comment has been minimized.

Copy link
Contributor

@mikela mikela commented Jan 15, 2020

@cyburgee If you don't mind temporary hacks you could add an akka.pattern.after before actually triggering the termination, something like:

val binding = Http().bindAndHandle(routes, "127.0.0.1", 8080)
shutdown.addTask(CoordinatedShutdown.PhaseServiceUnbind, "http-unbind") { () =>
  binding.flatMap(_.unbind()).map(_ => Done)
}
shutdown.addTask(CoordinatedShutdown.PhaseServiceRequestsDone, "http-graceful-termination") { () =>
  binding.flatMap(b =>
    akka.pattern.after(10.seconds, system.scheduler)(b.terminate(15.seconds))
  ).map(_ => Done)
}

And in the conf:

akka.coordinated-shutdown.default-phase-timeout = 15s

This will unbind the port and then enforce a "minimum delay" so all in-flight responses can return normally if they return in less than 10s.

The problems with this solution are that:

  1. Any request that takes longer than 10s will still have the problem you mentioned.
  2. The phase will take 10s even if there are no in-flight requests.
@hseeberger

This comment has been minimized.

Copy link
Contributor

@hseeberger hseeberger commented Jan 16, 2020

Instead of sleep you should use akka.pattern.after which already gives you a Future.

@mikela

This comment has been minimized.

Copy link
Contributor

@mikela mikela commented Jan 16, 2020

Thank you! Edited.

@jroper

This comment has been minimized.

Copy link
Contributor

@jroper jroper commented Jan 16, 2020

@cyburgee No, I haven't seen that, but I also haven't been using it in anger enough yet to see that. It sounds like what you're seeing is a bug. If you're able to reproduce, or can show more errors/interesting logs, it would be very helpful if you raised an issue about it.

@ignasi35

This comment has been minimized.

Copy link
Member

@ignasi35 ignasi35 commented Feb 2, 2020

Related to playframework/playframework#9970 (Play 2.8.1)

@raboof raboof added this to Incoming Issues and PRs in Akka streaming Feb 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Akka streaming
  
Incoming Issues and PRs
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.