Skip to content

Commit aa73d41

Browse files
DieBauerjenkins
authored and
jenkins
committed
[finagle-core]: Pass through configured deadline params rather than use default
Problem We can set the `TimeoutFilter.PropagateDeadlines` and `TimeoutFilter.PreferDeadlineOverTimeout` to configure behavior on the Client. When the MethodBuilder interface is used, the `perRequestModule` does only look at the `Default` values of these configurations, preventing custom configuration. Solution Pass the `TimeoutFilter.PropagateDeadlines` and `TimeoutFilter.PreferDeadlineOverTimeout` as Params to the `perRequestModule` to instantiate the `TimeoutFilter` with correct configuration. Result The MethodBuilder will pick up configured `TimeoutFilter.PropagateDeadlines` and `TimeoutFilter.PreferDeadlineOverTimeout` parameters. Closes #937 compile Differential Revision: https://phabricator.twitter.biz/D1107765
1 parent 610a21c commit aa73d41

File tree

3 files changed

+58
-11
lines changed

3 files changed

+58
-11
lines changed

CHANGELOG.rst

+2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ Bug Fixes
3636
3737
* finagle: Deposit budget once in MethodBuilder ``PHAB_ID=D1107653``
3838

39+
* finagle: Pass through configured deadline params rather than use default ``PHAB_ID=D1107765``
40+
3941

4042
22.12.0
4143
-------

finagle-core/src/main/scala/com/twitter/finagle/client/DynamicTimeout.scala

+18-11
Original file line numberDiff line numberDiff line change
@@ -95,31 +95,38 @@ object DynamicTimeout {
9595
* @see [[LatencyCompensation]]
9696
*/
9797
def perRequestModule[Req, Rep]: Stackable[ServiceFactory[Req, Rep]] =
98-
new Stack.Module3[
99-
TimeoutFilter.Param,
100-
param.Timer,
101-
LatencyCompensation.Compensation,
98+
new Stack.ModuleParams[
10299
ServiceFactory[Req, Rep]
103100
] {
104101
val role: Stack.Role = TimeoutFilter.role
105102
val description: String =
106103
"Apply a dynamic timeout-derived deadline to request"
107104

108-
def make(
109-
defaultTimeout: TimeoutFilter.Param,
110-
timer: param.Timer,
111-
compensation: LatencyCompensation.Compensation,
105+
val parameters = Seq(
106+
implicitly[Stack.Param[TimeoutFilter.Param]],
107+
implicitly[Stack.Param[param.Timer]],
108+
implicitly[Stack.Param[LatencyCompensation.Compensation]],
109+
implicitly[Stack.Param[TimeoutFilter.PropagateDeadlines]],
110+
implicitly[Stack.Param[TimeoutFilter.PreferDeadlineOverTimeout]]
111+
)
112+
113+
override def make(
114+
params: Stack.Params,
112115
next: ServiceFactory[Req, Rep]
113116
): ServiceFactory[Req, Rep] = {
117+
114118
val filter = new TimeoutFilter[Req, Rep](
115119
timeoutFn(
116120
PerRequestKey,
117-
defaultTimeout.tunableTimeout,
121+
params[TimeoutFilter.Param].tunableTimeout,
118122
TimeoutFilter.Param.Default, // tunableTimeout() should always produce a value,
119-
compensation.howlong // but we fall back on the default if not
123+
params[
124+
LatencyCompensation.Compensation].howlong // but we fall back on the default if not
120125
),
121126
duration => new IndividualRequestTimeoutException(duration),
122-
timer.timer
127+
params[param.Timer].timer,
128+
params[TimeoutFilter.PropagateDeadlines].enabled,
129+
params[TimeoutFilter.PreferDeadlineOverTimeout].enabled
123130
)
124131
filter.andThen(next)
125132
}

finagle-integration/src/test/scala/com/twitter/finagle/integration/MethodBuilderTest.scala

+38
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,15 @@ import com.twitter.finagle.server.StackServer
99
import com.twitter.finagle.util.DefaultTimer
1010
import com.twitter.finagle.mux
1111
import com.twitter.finagle._
12+
import com.twitter.finagle.service.TimeoutFilter.PropagateDeadlines
1213
import com.twitter.util.Await
1314
import com.twitter.util.Future
15+
1416
import java.net.InetSocketAddress
1517
import org.scalatest.funsuite.AnyFunSuite
1618

19+
import java.util.concurrent.atomic.AtomicBoolean
20+
1721
class MethodBuilderTest extends AnyFunSuite {
1822

1923
private def await[T](f: Future[T]): T = Await.result(f, 15.seconds)
@@ -153,4 +157,38 @@ class MethodBuilderTest extends AnyFunSuite {
153157
mux.Request.empty,
154158
mux.Response.empty
155159
)
160+
161+
test("Methodbuilder client does not propagate Deadlines") {
162+
val deadlinePresent = new AtomicBoolean(true)
163+
val service = Service.mk { request: http.Request =>
164+
deadlinePresent.set(
165+
request.headerMap.get("Finagle-Ctx-com.twitter.finagle.Deadline").isDefined)
166+
Future.value(http.Response())
167+
}
168+
169+
val server = Http.server
170+
.serve("localhost:*", service)
171+
val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
172+
173+
val noPropagationClient = Http.client
174+
.withLabel("backend-noprop")
175+
.configured(PropagateDeadlines(false).mk())
176+
.methodBuilder(s"${addr.getHostName}:${addr.getPort}")
177+
.newService
178+
179+
val defaultClient = Http.client
180+
.withLabel("backend")
181+
.methodBuilder(s"${addr.getHostName}:${addr.getPort}")
182+
.newService
183+
184+
await(noPropagationClient(http.Request("/")))
185+
assert(!deadlinePresent.get())
186+
await(defaultClient(http.Request("/")))
187+
assert(deadlinePresent.get())
188+
189+
await(server.close())
190+
await(noPropagationClient.close())
191+
await(defaultClient.close())
192+
}
193+
156194
}

0 commit comments

Comments
 (0)