Skip to content

Commit

Permalink
util-core: Remove configurability of Local state on Promise interrupts
Browse files Browse the repository at this point in the history
Problem

We have been using the Local state present when an interrupt handler was set
when a Promise has been raised on for some time now without issue.

Solution

Remove the associated code.

JIRA Issues: CSL-9421, PCM-110275

Differential Revision: https://phabricator.twitter.biz/D442444
  • Loading branch information
Bryce Anderson authored and jenkins committed Mar 2, 2020
1 parent 3db2808 commit da63894
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 50 deletions.
13 changes: 13 additions & 0 deletions CHANGELOG.rst
Expand Up @@ -7,6 +7,19 @@ Note that ``PHAB_ID=#`` and ``RB_ID=#`` correspond to associated messages in com
Unreleased
----------

Breaking API Changes
~~~~~~~~~~~~~~~~~~~~

* util-core: The system property `com.twitter.util.UseLocalInInterruptible` no longer
can be used to modulate which Local state is present when a Promise is interrupted.
``PHAB_ID=D442444``

Runtime Behavior Changes
~~~~~~~~~~~~~~~~~~~~~~~~

* util-core: Promises now exclusively use the state local to setting the interrupt
handler when raising on a Promise. ``PHAB_ID=D442444``

20.2.1
------

Expand Down
24 changes: 5 additions & 19 deletions util-core/src/main/scala/com/twitter/util/Promise.scala
Expand Up @@ -7,16 +7,6 @@ import scala.util.control.NonFatal

object Promise {

// An experimental property that enables the interrupt handler
// to use the state at the time of callback creation.
@volatile private var UseLocalInInterruptible: Boolean =
System.getProperty("com.twitter.util.UseLocalInInterruptible", "false").toBoolean

// Switches to enabling the interrupt handler to use
// the state at the time of callback creation.
private[twitter] def useLocalInInterruptible(useLocal: Boolean): Unit =
UseLocalInInterruptible = useLocal

/**
* Embeds an "interrupt handler" into a [[Promise]].
*
Expand Down Expand Up @@ -579,15 +569,11 @@ class Promise[A] extends Future[A] with Promise.Responder[A] with Updatable[Try[
case s: Interruptible[A] =>
if (!cas(s, new Interrupted(s.waitq, intr))) raise(intr)
else {
if (!UseLocalInInterruptible)
s.handler.applyOrElse(intr, Promise.AlwaysUnit)
else {
val current = Local.save()
if (current ne s.saved)
Local.restore(s.saved)
try s.handler.applyOrElse(intr, Promise.AlwaysUnit)
finally Local.restore(current)
}
val current = Local.save()
if (current ne s.saved)
Local.restore(s.saved)
try s.handler.applyOrElse(intr, Promise.AlwaysUnit)
finally Local.restore(current)
}

case s: Transforming[A] =>
Expand Down
31 changes: 0 additions & 31 deletions util-core/src/test/scala/com/twitter/util/PromiseTest.scala
Expand Up @@ -220,8 +220,6 @@ class PromiseTest extends FunSuite {
}

test("interrupts use local state at the time of callback creation") {
Promise.useLocalInInterruptible(true)

val local = new Local[String]
implicit val timer: Timer = new JavaTimer(true)

Expand All @@ -246,33 +244,6 @@ class PromiseTest extends FunSuite {
assert("main thread" == Await.result(f, 3.seconds))
}

test("interrupts use raisers state") {
Promise.useLocalInInterruptible(false)

val local = new Local[String]
implicit val timer: Timer = new JavaTimer(true)

local.set(Some("main thread"))
val p = new Promise[String]

p.setInterruptHandler({
case _ =>
val localVal = local().getOrElse("<empty>")
p.updateIfEmpty(Return(localVal))
})
val f = Future.sleep(1.second).before(p)

val thread = new Thread(new Runnable {
def run(): Unit = {
local.set(Some("worker thread"))
p.raise(new Exception)
}
})
thread.start()

assert("worker thread" == Await.result(f, 3.seconds))
}

test("interrupt then map uses local state at the time of callback creation") {
val local = new Local[String]
implicit val timer: Timer = new JavaTimer(true)
Expand Down Expand Up @@ -302,8 +273,6 @@ class PromiseTest extends FunSuite {
}

test("locals in Interruptible after detatch") {
Promise.useLocalInInterruptible(true)

val local = new Local[String]
implicit val timer: Timer = new JavaTimer(true)

Expand Down

0 comments on commit da63894

Please sign in to comment.