Sitelet https://github.com/twitter/util/commit/b152745ef5d06d9b688d330fb7d30cbd7a0fd2cc
Skip to content

Commit b152745

Browse files
cacocojenkins
cacoco
authored and
jenkins
committed
util-app: Add visibility for NonFatal exceptions during exiting of an App
Summary: Problem When exceptions occur in executing `c.t.app.App#close` they are mostly swallowed -- the `closeOnExitLast` returns the first encountered exception. However, there is no visibilty on any NonFatal exception from executing registered `onExit` functions or closing `closeOnExit` Closables, nor over all NonFatals from closing `closeOnExitLast` Closables. Solution Update the logic in `c.t.app.App#close` to allow for logging and collecting NonFatal exceptions which occur during closing of the app. The collected exceptions are finally thrown in the new CloseException. Result Users will get visibility into NonFatal exceptions which happen during closing of the application through logging and collection of causes in a thrown CloseException. JIRA Issues: CSL-6040 Differential Revision: https://phabricator.twitter.biz/D146029
1 parent 0c793f9 commit b152745

4 files changed

Lines changed: 156 additions & 13 deletions

File tree

‎CHANGES‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,10 @@ Unreleased
88

99
Runtime Behavior Changes:
1010

11+
* util-app: Add visibility for NonFatal exceptions during exiting of `c.t.app.App`.
12+
Added visibility into any NonFatal exceptions which occur during the closing of
13+
resources during `App#close`. ``PHAB_ID=D146029``
14+
1115
* util-core: Ensure the `Awaitable.CloseAwaitably0.closeAwaitably` Future returns.
1216
Because the `closed` AtomicBoolean is flipped, we want to make sure that executing
1317
the passed in `f` function satisfies the `onClose` Promise even the cases of thrown

‎util-app/src/main/scala/com/twitter/app/App.scala‎

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,20 @@ trait App extends Closable with CloseAwaitably {
6161
*/
6262
protected def failfastOnFlagsNotParsed: Boolean = false
6363

64+
/** Exit on error with the given Throwable */
65+
protected def exitOnError(throwable: Throwable): Unit = {
66+
throwable.printStackTrace()
67+
throwable match {
68+
case _: CloseException =>
69+
// exception occurred while closing, do not attempt to close again
70+
System.err.println(throwable.getMessage)
71+
System.exit(1)
72+
case _ =>
73+
exitOnError("Exception thrown in main on startup")
74+
}
75+
}
76+
77+
/** Exit on error with the given `reason` String */
6478
protected def exitOnError(reason: String): Unit = {
6579
exitOnError(reason, "")
6680
}
@@ -191,21 +205,54 @@ trait App extends Closable with CloseAwaitably {
191205
final def close(deadline: Time): Future[Unit] = synchronized {
192206
closing = closeAwaitably {
193207
closeDeadline = deadline.max(Time.now + MinGrace)
194-
val firstPhase = Closable
195-
.all(exits.asScala.toSeq: _*)
196-
.close(closeDeadline)
208+
Future
209+
.collectToTry(exits.asScala.toSeq.map(_.close(closeDeadline)))
197210
.by(shutdownTimer, closeDeadline)
198-
199-
firstPhase
200-
.transform { _ =>
201-
Closable.all(lastExits.asScala.toSeq: _*).close(closeDeadline)
211+
.transform {
212+
case Return(results) =>
213+
closeLastExits(results, closeDeadline)
214+
case Throw(t) =>
215+
// this would be triggered by a timeout on the collectToTry of exits,
216+
// still try to close last exits
217+
closeLastExits(Seq(Throw(t)), closeDeadline)
202218
}
203-
.by(shutdownTimer, closeDeadline)
204219
}
205-
206220
closing
207221
}
208222

223+
private[this] def newCloseException(errors: Seq[Throwable]): CloseException = {
224+
val message = if (errors.size == 1) {
225+
"An error occurred on exit"
226+
} else {
227+
s"${errors.size} errors occurred on exit"
228+
}
229+
val exc = new CloseException(message)
230+
errors.foreach { error =>
231+
exc.addSuppressed(error)
232+
}
233+
exc
234+
}
235+
236+
private[this] final def closeLastExits(
237+
onExitResults: Seq[Try[Unit]],
238+
deadline: Time
239+
): Future[Unit] = {
240+
Future
241+
.collectToTry(lastExits.asScala.toSeq.map(_.close(deadline)))
242+
.by(shutdownTimer, deadline)
243+
.transform {
244+
case Return(results) =>
245+
val errors = (onExitResults ++ results).collect { case Throw(e) => e }
246+
if (errors.isEmpty) {
247+
Future.Done
248+
} else {
249+
Future.exception(newCloseException(errors))
250+
}
251+
case Throw(t) =>
252+
Future.exception(newCloseException(Seq(t)))
253+
}
254+
}
255+
209256
final def main(args: Array[String]): Unit = {
210257
try {
211258
nonExitingMain(args)
@@ -214,9 +261,8 @@ trait App extends Closable with CloseAwaitably {
214261
exitOnError(reason)
215262
case FlagParseException(reason, _) =>
216263
exitOnError(reason, flag.usage)
217-
case e: Throwable =>
218-
e.printStackTrace()
219-
exitOnError("Exception thrown in main on startup")
264+
case t: Throwable =>
265+
exitOnError(t)
220266
}
221267
}
222268

‎util-app/src/main/scala/com/twitter/app/CloseException.scala‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package com.twitter.app
2+
3+
import scala.util.control.NoStackTrace
4+
5+
/**
6+
* An exception that represents collected errors which occurred on close of the app.
7+
*
8+
* @note When execution of the `App#nonExitingMain` throws a [[CloseException]], the app will not
9+
* attempt to call `App#close()` again in the `App#exitOnError(t: Throwable)` function since
10+
* this Exception is assumed be a result of already calling `App#close()`.
11+
*
12+
* @note Collected exceptions which occurred during closing are added as "suppressed" exceptions.
13+
*
14+
* @see [[https://docs.oracle.com/javase/7/docs/api/java/lang/Throwable.html#getSuppressed()]]
15+
*/
16+
final class CloseException private[twitter] (message: String)
17+
extends Exception(message)
18+
with NoStackTrace

‎util-app/src/test/scala/com/twitter/app/AppTest.scala‎

Lines changed: 76 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
package com.twitter.app
22

33
import com.twitter.conversions.time._
4-
import com.twitter.util.{Closable, Future, MockTimer, Promise, Time, Timer}
4+
import com.twitter.util._
55
import java.util.concurrent.ConcurrentLinkedQueue
66
import org.scalatest.FunSuite
7+
import scala.language.reflectiveCalls
78

89
class TestApp(f: () => Unit) extends App {
910
var reason: Option[String] = None
@@ -24,6 +25,14 @@ object VeryBadApp extends App {
2425
def main(): Unit = {}
2526
}
2627

28+
trait ErrorOnExitApp extends App {
29+
override val defaultCloseGracePeriod: Duration = 2.seconds
30+
31+
override def exitOnError(throwable: Throwable): Unit = {
32+
throw throwable
33+
}
34+
}
35+
2736
class AppTest extends FunSuite {
2837
test("App: make sure system.exit called on exception from main") {
2938
val test1 = new TestApp(() => throw new RuntimeException("simulate main failing"))
@@ -127,6 +136,7 @@ class AppTest extends FunSuite {
127136

128137
ctl.advance(2.seconds)
129138
t.tick()
139+
t.tick()
130140

131141
assert(n2 == 1)
132142
assert(f.isDefined)
@@ -177,6 +187,7 @@ class AppTest extends FunSuite {
177187

178188
ctl.advance(2.seconds)
179189
t.tick()
190+
t.tick()
180191

181192
assert(n1 == 1)
182193
assert(n2 == 1)
@@ -228,6 +239,7 @@ class AppTest extends FunSuite {
228239

229240
ctl.advance(2.seconds)
230241
t.tick()
242+
t.tick()
231243

232244
assert(n1 == 1)
233245
assert(n2 == 1)
@@ -269,4 +281,67 @@ class AppTest extends FunSuite {
269281
app.closeOnExit(closable)
270282
assert(closed)
271283
}
284+
285+
test("App: exit functions properly capture non-fatal exceptions") {
286+
val app = new ErrorOnExitApp {
287+
def main(): Unit = {
288+
onExit{
289+
throw new Exception("FORCED ON EXIT")
290+
}
291+
292+
closeOnExit(Closable.make { _ =>
293+
throw new Exception("FORCED CLOSE ON EXIT")
294+
})
295+
296+
closeOnExitLast(Closable.make { _ =>
297+
throw new Exception("FORCED CLOSE ON EXIT LAST")
298+
})
299+
}
300+
}
301+
302+
val e = intercept[CloseException] {
303+
app.main(Array.empty)
304+
}
305+
306+
assert(e.getSuppressed.length == 3)
307+
}
308+
309+
test("App: fatal exceptions escape exit functions") {
310+
// first fatal (InterruptedException) kills the app during close
311+
val app = new ErrorOnExitApp {
312+
def main(): Unit = {
313+
closeOnExit(Closable.make { _ =>
314+
throw new InterruptedException("FORCED CLOSE ON EXIT")
315+
})
316+
}
317+
}
318+
319+
intercept[InterruptedException] {
320+
app.main(Array.empty)
321+
}
322+
}
323+
324+
test("App: exit functions properly capture mix of non-fatal and fatal exceptions") {
325+
val app = new ErrorOnExitApp {
326+
def main(): Unit = {
327+
onExit{
328+
throw new Exception("FORCED ON EXIT")
329+
}
330+
331+
closeOnExit(Closable.make { _ =>
332+
throw new Exception("FORCED CLOSE ON EXIT")
333+
})
334+
335+
closeOnExitLast(Closable.make { _ =>
336+
throw new InterruptedException("FORCED CLOSE ON EXIT LAST")
337+
})
338+
}
339+
}
340+
341+
val e = intercept[Throwable] {
342+
app.main(Array.empty)
343+
}
344+
345+
assert(e.getClass == classOf[InterruptedException])
346+
}
272347
}

0 commit comments

Comments
 (0)