NoContent status code ignores failure of Future #589

Closed
curreli opened this Issue Nov 28, 2016 · 9 comments

Projects

Done in Bug Hunting

5 participants

@curreli
curreli commented Nov 28, 2016

I recently upgraded to akka-http 10.0.0 from 2.4.x and my API started returning successes when it should actually fail. I tracked it down to the NoContent status code which when used, ignores the result of the Future.

complete(NoContent, doSomethingWhichReturnsAFuture())

If doSomethingWhichReturnsAFuture() fails, i.e. if the Future returns a scala.util.Failure, the server still returns a 204 success response. If I change NoContent to OK then the failed future is transformed into an error, as expected.

Can you advise on whether this is a regression or a planned change? In the latter case, how can I go back to the previous behavior?

Thank you!

@curreli curreli changed the title from NoContent return type ignores failure of Future to NoContent status code ignores failure of Future Nov 28, 2016
@jonas
Contributor
jonas commented Dec 5, 2016 edited

I bisected this using git bisect master b8dd16b751daa9d42ff7da73ae878dae7501dda6 and the following test:

class NoContent589Spec extends RoutingSpec {
  "#589" in {
    def doSomethingWhichReturnsAFuture(): Future[String] =
      Future.failed(new Exception("oops"))

    val route =
      path("589") {
        complete((NoContent, doSomethingWhichReturnsAFuture))
      }

    Get("/589") ~> route ~> check { responseAs[String] shouldEqual "There was an internal server error." } // hide
  }
}

The behaviour was changed by @ktoso in 0ad8017 which results in the following failure:

[info] - #589 *** FAILED ***
[info]   "[]" did not equal "[There was an internal server error.]" (NoContent589Spec.scala:23)

It looks like the following lines caused the breakage. Unless someone beats me to it, I will take a closer look tomorrow.

diff --git a/akka-http/src/main/scala/akka/http/scaladsl/marshalling/PredefinedToResponseMarshallers.scala b/akka-http/src/main/scala/akka/http/scaladsl/marsh
index 49ff565..d587566 100644
--- a/akka-http/src/main/scala/akka/http/scaladsl/marshalling/PredefinedToResponseMarshallers.scala
+++ b/akka-http/src/main/scala/akka/http/scaladsl/marshalling/PredefinedToResponseMarshallers.scala
@@ -47,7 +57,8 @@ trait PredefinedToResponseMarshallers extends LowPriorityToResponseMarshallerImp

   implicit def fromStatusCodeAndHeadersAndValue[T](implicit mt: ToEntityMarshaller[T]): TRM[(StatusCode, immutable.Seq[HttpHeader], T)] =
     Marshaller(implicit ec ⇒ {
-      case (status, headers, value) ⇒ mt(value).fast map (_ map (_ map (HttpResponse(status, headers, _))))
+      case (status, headers, value) if (status.allowsEntity) ⇒ mt(value).fast map (_ map (_ map (HttpResponse(status, headers, _))))
+      case (status, headers, _)                              ⇒ fromStatusCodeAndHeaders((status, headers))
     })

   implicit def fromEntityStreamingSupportAndByteStringMarshaller[T, M](implicit s: EntityStreamingSupport, m: ToByteStringMarshaller[T]): ToResponseMarshalle
@jonas
Contributor
jonas commented Dec 5, 2016

Correction: it was changed by @gosubpl and forward ported from akka/akka by @ktoso in #297

@jrudolph jrudolph added this to the 10.0.1 milestone Dec 5, 2016
@jrudolph
Member
jrudolph commented Dec 5, 2016

Great analysis, @jonas.

However, I wonder if the old behavior was more correct than the new one? Why would you first marshal something to an entity and then combine it with a NoContent status code that disallows an entity?

@curreli can you explain your use case for that pattern?

An alternative could be:

onSuccess(doSomethingWhichReturnsAFuture()) { _ => // ignore value
  complete(NoContent)
}
@gosubpl
Contributor
gosubpl commented Dec 6, 2016

Yes, indeed @jrudolph I think it would be good to pinpoint the desired behaviour. In any case I will look into this around the week-end.

@gosubpl
Contributor
gosubpl commented Dec 16, 2016

@curreli @jrudolph @jonas as I understand from akka/akka#20793 what currently is implemented meets the requirements specified by @stjohnb .

Please think for a while, what options of semantics are available when we run this line:

complete(NoContent, doSomethingWhichReturnsAFuture())
  1. Evaluate the future in dSomethingWhichReturnsAFuture and see whether this was a Success or Failure
  2. In case of Success ignore the generated entity and return NoContent (we shouldn't put any content in the reply, see below) - why did we need to evaluate this in first place, as no content is going to be returned?
  3. In case of Failure do either:
    3.1 return NoContent without any content (which would be what happens currently)
    3.2 return NoContent with a content notifying of a failure (which would violate RFC stating a 204 response is terminated by the first empty line after the header fields because it cannot contain a message body.)
    3.3 return InternalServerError which is what happens for complete(StatusCodes.OK, ...

Only 3.3 seems to be a viable option for changing the behaviour, as 3.1 is what happens currently and 3.2 is disallowed by RFC. Which, if I understand correctly, is what @curreli requests.
That fix should not be difficult. However, the problem is that putting StatusCodes.NoContent in the complete( directive is usually done in situations where the programmer wants to make sure that no content will be generated. So before changing the behaviour to described by 3.3 I'd like people like @curreli and @stjohnb to chime in and give more background as to the context in which complete(StatusCodes.NoContent is being used... e.g. why run a function completing the response where what we intend to do is to return no content at all.

@jrudolph jrudolph modified the milestone: 10.0.1, 10.0.x Dec 22, 2016
@curreli
curreli commented Dec 26, 2016 edited

@gosubpl Hey, sorry for the delay!

My use case is simple, I return NoContent when deleting an object but I want the 204 success code to be returned only if the deletion was successful. This works with all other success codes so why shouldn't it work with 204? It would be wrong to return a success code if my object did not get deleted! I agree that 3.3 is the best option and I wouldn't expect anything else. Note that this was the case before.

For now I fixed this by creating a helper method:

protected def completeWithNoContent(f: Future[Unit]) = onSuccess(f) {
  complete(NoContent)
}

And by not using the natural complete(NoContent, ...) anymore.

@jrudolph
Member

Thanks for breaking the solutions down, @gosubpl. In summary, I think we agree that 3.3 would be the way to go forward.

@gosubpl gosubpl added a commit to gosubpl/akka-http that referenced this issue Jan 2, 2017
@gosubpl gosubpl =htc don't ignore failed future for NoContent responses (#589) bdddcb0
@gosubpl gosubpl added a commit to gosubpl/akka-http that referenced this issue Jan 2, 2017
@gosubpl gosubpl spelling fix (#589) 8bf715e
@gosubpl gosubpl added a commit to gosubpl/akka-http that referenced this issue Jan 2, 2017
@gosubpl gosubpl =htc don't ignore failed future for NoContent responses (#589) 987a1da
@gosubpl
Contributor
gosubpl commented Jan 2, 2017

My bad, should have considered this use case. @curreli @jrudolph #703 should fix the regression.

@ktoso ktoso closed this Jan 2, 2017
@ktoso
Member
ktoso commented Jan 2, 2017

Thanks for the fix @gosubpl

@ktoso ktoso modified the milestone: 10.0.2, 10.0.x Jan 2, 2017
@ktoso ktoso removed the 1 - triaged label Jan 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment