Skip to content

Commit

Permalink
finagle-http: Remove Use of request.response from ExceptionFilter
Browse files Browse the repository at this point in the history
Problem / Solution

Use of 'request.response' within Finagle is an anti-pattern. Let's
remove its use from finagle-http's `ExceptionFilter`.

JIRA Issues: CSL-8391

Differential Revision: https://phabricator.twitter.biz/D333509
  • Loading branch information
ryanoneill authored and jenkins committed Jun 27, 2019
1 parent 232e36b commit 54d4acf
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 12 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ Runtime Behavior Changes
* finagle-http: `c.t.f.http.Cors` has been changed to no longer use the `c.t.f.http.Response`
associated with the passed in `c.t.f.http.Request`. ``PHAB_ID=D332765``

* finagle-http: `c.t.f.http.filter.ExceptionFilter` has been changed to no longer
use the `c.t.f.http.Response` associated with the passed in `c.t.f.http.Request`.
``PHAB_ID=D333509``

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@ class ExceptionFilter[REQUEST <: Request] extends SimpleFilter[REQUEST, Response
case e: CancelledRequestException =>
// This only happens when ChannelService cancels a reply.
log.warning("cancelled request: uri:%s", request.uri)
respond(request, ClientClosedRequestStatus)
respond(ClientClosedRequestStatus)
case e: Throwable =>
try {
log.warning(e, "exception: uri:%s exception:%s", request.uri, e)
respond(request, Status.InternalServerError)
respond(Status.InternalServerError)
} catch {
// logging or internals are broken. Write static string to console -
// don't attempt to include request or exception.
Expand All @@ -44,13 +44,8 @@ class ExceptionFilter[REQUEST <: Request] extends SimpleFilter[REQUEST, Response
}
}

private def respond(request: REQUEST, responseStatus: Status): Future[Response] = {
val response = request.response
response.status = responseStatus
response.clearContent()
response.contentLength = 0
Future.value(response)
}
private def respond(responseStatus: Status): Future[Response] =
Future.value(Response(responseStatus))
}

object ExceptionFilter extends ExceptionFilter[Request] {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class ExceptionFilterTest extends FunSuite {

assert(response.status == Status.InternalServerError)
assert(response.contentString == "")
assert(response.contentLength == Some(0))
assert(response.contentLength == None)
}

test("handle throw") {
Expand All @@ -50,7 +50,7 @@ class ExceptionFilterTest extends FunSuite {

assert(response.status == Status.InternalServerError)
assert(response.contentString == "")
assert(response.contentLength == Some(0))
assert(response.contentLength == None)
}

test("handle cancel") {
Expand All @@ -59,6 +59,6 @@ class ExceptionFilterTest extends FunSuite {

assert(response.statusCode == 499)
assert(response.contentString == "")
assert(response.contentLength == Some(0))
assert(response.contentLength == None)
}
}

0 comments on commit 54d4acf

Please sign in to comment.