Add BracketRequestResponse Middleware

[isomarcte]

This middleware allows for expressing bracketing operations, beginning at the reception of a Request, and ending when the Response body is fully consumed, handling all error branches. It is semantically the same as Bracket from cats.effect. Because http4s defines an http services as Kleisli[OptionT[F, ?], Request[F], Response[F]], you can't actually use any existing bracketing semantics from cats-effect or fs2 out of the box to bracket over the acquisition of the Request and the consumption of the Response body.

Code of this structure, or very similar to it, already exists in http4s, but not generically. By implementing it generically, we are able to remove a great deal of code as well as make the semantics more clear.

In addition to BracketRequestResponse, this commit contains the following other changes.

• A new middleware is added named ConcurrentRequests. This middleware takes functions to run at the start and end of a request/response interaction which report the current value of the concurrent requests for the service.

The following middlewares were re-written in terms of BracketRequestResponse.

• MaxActiveRequests
  • Technically, this was re-written in terms of ConcurrentRequests.
  • As a result of the re-write, the constraint can be dropped from Concurrent to Sync. This may seem alarming, but it is actually safe. No asynchronous or cancellation is required to merely track the number of concurrent requests. The former implementation derived the Concurrent constraint from the use of a Semaphore, but since ConcurrentRequests only requires a Ref, only Sync is needed.
• Metrics
  • This yielded a significant reduction in code and (subjectively) clarified the semantics.
  • The new implementation adheres to the old semantics, with the following exceptions.
    • decreaseActiveRequests is now run before the other metrics operations, formerly it was run after. This may be why there have been some reported instances of the active requests metric getting stuck at non-zero value.
    • recordHeadersTime is no longer called when the body is consumed, only when the Response (and thus the headers) are generated. This appears to have been a bug in the previous implementation.
    • When an error occurs in generation the Response, we only poll the Clock one time and use that same value to report the elapsed time for both the recordHeadersTime and recordAbnormalTermination. The previous implementation polled twice in this case, even though they should effectively be occurring at the same time. See line 100 and 165 in the previous commit.
      • Note: This is why the tests were updated. The tests assumed, via FakeClock, that there would be two polling events in the error branch.
    • In the former implementation, metrics are only recorded on error if errorResponseHandler is yields a non-empty result. This includes all metrics, e.g. recordHeadersTime, recordTotalTime, and recordAbnormalTermination. This seems to be a result of the fact that recordTotalTime requires a Status value, which will not be available if errorResponseHandler yields None. Since MetricsOps is unchanged, this is still the case for invoking recordTotalTime, but we can invoke recordAbnormalTermination and recordHeadersTime even if errorResponseHandler yields None, so the implementation does this.

[rossabaker]

Wow.

It's too late in my day for me to give this the thought it deserves, but let me double check that I understand the strategy before diving in tomorrow: this won't save us from backends not draining the response body like #2663 would, but it centralizes all the workarounds we've implemented in various middlewares to simulate it?

[rossabaker]

Wow.

To clarify, that's a "Wow, impressive work", not a "Wow, WTF". 😄

[isomarcte]

this won't save us from backends not draining the response body like #2663 would, but it centralizes all the workarounds we've implemented in various middlewares to simulate it?

That is correct.

Additionally, one could trivially bypass the release on body stream termination by discarding the response body, if they do so outside the scope of the middleware. This was the case before this commit as well.

I've some half ideas on how we might be able to ensure finalizers run, even if the response body or even the whole response gets discarded, but that is beyond the scope of this PR (plus they may not even work, like I said "half ideas").

[isomarcte]

Additionally, one could trivially bypass the release on body stream termination by discarding the response body, if they do so outside the scope of the middleware. This was the case before this commit as well.

I should probably make a note about that edge case in the comments.

[isomarcte]

@rossabaker I added a comment about that edge case.

[rossabaker]

I like it. Leaning into ContextRequest instead of attributes avoids some options, but I wonder whether it impairs composability with other things that might already be a ContextRequest. We don't have a good way of nesting contexts yet. Maybe that's not a problem in practice, or anything we can solve in series/0.21, but something to think about.

[rossabaker]

I think there are still bigger thoughts to be had about this and ContextRequest and the old idea of turning message into a typeclass like @ChristopherDavenport has been proposing forever, but that's a 1.0 discussion.

[rossabaker]

I think we called this 2 instead of _ when we did similar in Http4sDsl.

[isomarcte]

@rossabaker I'll update. _ is my default ' Haskell idioms. I never know what to name things.

[isomarcte]

updated

[rossabaker]

I think of the _ as being unit, as in traverse_, but that's possibly just Cats looking for an ' as well.

[rossabaker]

I think maybe I was expecting to see an fs2.Signal on this instead of those callbacks. I don't have a feel for whether that's easier or harder to work with.

[rossabaker]

We use this name in a lot of places, so if we're moving this direction, we should follow up and do it in more places.

[isomarcte]

@rossabaker I actually like the other name just fine, but I needed a fresh name due to the binary compatibility change. I'm certainly open to suggestions on how to go about naming this.

[rossabaker]

Oh, the Concurrent => Sync. You mentioned that, and then I didn't spot it. No, I think you did the right thing.

[rossabaker]

I would like to rethink context request and response, and how they compose, and how they complement and contrast vault. I think there's a long design discussion to be had around this.

But within the constraints of 0.21, this seems reasonable and likely to fix some issues.

[isomarcte]

@rossabaker I'm a bit confused here. I removed a comment, and now CI is complaining about files not having headers...but they definitely have headers...

A local sbt clean scalafmtAll scalafixAll test yielded no changes to the source tree. Thoughts?

[isomarcte]

Rebase on the latest series/0.21 also did nothing.

[nigredo-tori]

There should probably be a version of this that works in terms of Resource. All the resources I would want to use with this middleware come as Resources.

[isomarcte]

@nigredo-tori Are you suggesting a variant where the F is Resource?

[rossabaker]

There's no reason other than More Names this can't be added, right?

[nigredo-tori]

I'm suggesting something like

def bracketRequestResponseResourceRoutes[F[_], A](
    resource: Resource[F, A]
)(implicit F: Bracket[F, Throwable]): ContextMiddleware[F, A]

Resource wraps a pair of acquire and release. Libraries generally expose Resources other than those, so it makes sense to provide support for this use case.

[isomarcte]

@nigredo-tori

Unless I'm misunderstanding you (which is entirely possible) the only possible implementation of that signature which makes sense is something like this,

  // I've not compile this, but I think it is correct

  def bracketRequestResponseResourceRoutes[F[_], A](
    resource: Resource[F, A]
  )(implicit F: Bracket[F, Throwable]): ContextMiddleware[F, A] = {
    (bracketRoutes: Kleisli[OptionT[F, *], ContextRequest[F], Response[F]]) =>
    Kleisli((request: Request[F]) => 
      resource.use(a =>
        bracketRoutes.run(ContextRequest(a, resquest))
      )
    )
  }

I'd be very happy to add that, but I want to make sure those semantics are what you want. The release function of the Resource will be run when the effect to generate the Response[F], but before Response.body: Stream[F, Byte] is run. Is that what you are asking for?

My original indent for the BracketRequestResponse middleware was to be able to attach a finalizer to termination of the Response.body xor the F[Option[Response[F]]] if the latter was empty or failed. Unfortunately there is no way to write that logic using Resource with the current encodings. This is because the F[Option[Response[F]]] doesn't currently compose with the Response.body: Stream[F, Byte] scope.

Let me know if I'm just misunderstanding you.

[nigredo-tori]

I was thinking more along these lines:

(contextRoutes: ContextRoutes[F, A]) => {
  val contextRoutes0: ContextRoutes[F, (A, F[Unit])] =
    contextRoutes.contramap(_.map(_._1))
  bracketRequestResponseRoutes[F, (A, F[Unit])](resource.allocated)(_._2)(contextRoutes0)
}

I haven't compiled it, but the types should align. Resource.allocated is the key here - it transforms a Resource into something compatible with the acquire/release shape.

This would be sloppy with ExitCase handling, though, since allocated always releases with ExitCase.Completed. We might want to first add some kind of Resource.allocatedCase to cats-effect.

[isomarcte]

Ah, that makes more sense. I didn't realize that Resource exposed a function like allocated.

[rossabaker]

I rolled back our customization of the copyright headers in favor of the default when we adopted sbt-spiewak, and after I merged that, CI is testing your files on the merge commit. I fixed it with headerCheckAll. This might hit any PRs in flight that added files.

[isomarcte]

@rossabaker thanks for taking a look!