Skip to content

Commit

Permalink
[twitter.server] offload ShutdownHandler
Browse files Browse the repository at this point in the history
# Problem
ShutdownHandler is executed on a netty IO thread and calls app.close which can easily block this thread.

# Solution
Offload execution to a thread.

JIRA Issues: STOR-1705

Differential Revision: https://phabricator.twitter.biz/D1109579
  • Loading branch information
Anton Ivanov authored and jenkins committed Nov 10, 2023
1 parent 9e3d2e6 commit 12ff1d8
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@ package com.twitter.server.handler

import com.twitter.app.App
import com.twitter.finagle.Service
import com.twitter.finagle.http.{Method, Request, Response, Status, Uri}
import com.twitter.finagle.http.Method
import com.twitter.finagle.http.Request
import com.twitter.finagle.http.Response
import com.twitter.finagle.http.Status
import com.twitter.finagle.http.Uri
import com.twitter.io.Buf
import com.twitter.server.util.HttpUtils.{newOk, newResponse}
import com.twitter.server.util.HttpUtils.newOk
import com.twitter.server.util.HttpUtils.newResponse
import com.twitter.util.logging.Logger
import com.twitter.util.{Duration, Future}
import com.twitter.util.Duration
import com.twitter.util.Future
import com.twitter.util.Local

class ShutdownHandler(app: App) extends Service[Request, Response] {
private[this] val log = Logger[ShutdownHandler]
Expand Down Expand Up @@ -38,7 +45,22 @@ class ShutdownHandler(app: App) extends Service[Request, Response] {
s"[${req.uri}] from ${req.remoteAddress.getHostAddress} " +
s"quitting with grace period $grace"
)
app.close(grace)

// Isolate a current thread (likely netty IO thread)
// from probable blocking calls inside app.close.
val ctx = Local.save()
new Thread(
() =>
try {
Local.restore(ctx)
app.close(grace)
} catch {
case th: Throwable =>
log.error("Exception when asking an app to close", th)
},
"shutdown-handler" // thread name
).start()

newOk("quitting\n")
} else {
log.info(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
package com.twitter.server.handler

import com.twitter.conversions.DurationOps._
import com.twitter.finagle.http.{Method, Request, Status}
import com.twitter.finagle.http.Method
import com.twitter.finagle.http.Request
import com.twitter.finagle.http.Status
import com.twitter.server.TwitterServer
import com.twitter.util.{Await, Awaitable, Closable, Duration, Future, Time}
import com.twitter.util.Await
import com.twitter.util.Awaitable
import com.twitter.util.Closable
import com.twitter.util.Duration
import com.twitter.util.Future
import com.twitter.util.Time
import org.scalatest.concurrent.Eventually.eventually
import org.scalatest.concurrent.PatienceConfiguration.Timeout
import org.scalatest.funsuite.AnyFunSuite
import org.scalatest.time.Seconds
import org.scalatest.time.Span

class ShutdownHandlerTest extends AnyFunSuite {

Expand Down Expand Up @@ -38,7 +49,9 @@ class ShutdownHandlerTest extends AnyFunSuite {
val handler = new ShutdownHandler(closer)
val rsp = await(handler(Request(Method.Post, "/foo")))
assert(rsp.status == Status.Ok)
assert(closer.closed)
eventually(Timeout(Span(5, Seconds))) {
assert(closer.closed)
}
})

test("close without a grace period, but closer overrides default grace")(
Expand All @@ -53,7 +66,9 @@ class ShutdownHandlerTest extends AnyFunSuite {
val handler = new ShutdownHandler(closer)
val rsp = await(handler(Request(Method.Post, "/foo")))
assert(rsp.status == Status.Ok)
assert(closer.closed)
eventually(Timeout(Span(5, Seconds))) {
assert(closer.closed)
}
})

test("close with a grace period") {
Expand All @@ -66,7 +81,9 @@ class ShutdownHandlerTest extends AnyFunSuite {
val handler = new ShutdownHandler(closer)
val rsp = await(handler(Request(Method.Post, "/foo?grace=" + grace.toString)))
assert(rsp.status == Status.Ok)
assert(closer.closed)
eventually(Timeout(Span(5, Seconds))) {
assert(closer.closed)
}
}

test("fail when an invalid grace parameter is specified") {
Expand Down

0 comments on commit 12ff1d8

Please sign in to comment.