Drain response body in DefaultClient#defaultOnError
#6376
Conversation
Can we pattern match on the constraint, to see if it is in fact |
| override def defaultOnError(req: Request[F])(resp: Response[F])(implicit | ||
| G: Applicative[F] | ||
| ): F[Throwable] = | ||
| resp.body.compile.drain.as(UnexpectedStatus(resp.status, req.method, req.uri)) |
There was a problem hiding this comment.
Reminder: in main this should be like
override def defaultOnError(req: Request[F])(resp: Response[F])(implicit
G: Applicative[F]
): F[Throwable] =
resp.entity match {
case Entity.Empty | Entity.Strict(_) =>
F.pure(UnexpectedStatus(resp.status, req.method, req.uri))
case Entity.Default(body, _) =>
body.compile.drain.as(UnexpectedStatus(resp.status, req.method, req.uri))
}|
@armanbilge oh, indeed, thanks. Though I don't that much like this approach, however it works and uses many times in Typelevel libraries. |
|
Pushing this pattern forward, we could apply it even in |
| def defaultOnError(req: Request[F])(resp: Response[F])(implicit | ||
| F: Applicative[F] | ||
| ): F[Throwable] = | ||
| F.pure(UnexpectedStatus(resp.status, req.method, req.uri)) |
There was a problem hiding this comment.
Ah, right. I was thinking we would fix this in DefaultClient instead of for the backends independently. I remember looking at the PR you linked maybe a week ago, but I didn't have this idea then.
| resp.body.compile.drain.as(UnexpectedStatus(resp.status, req.method, req.uri)) | ||
|
|
||
| case _ => | ||
| F.pure(UnexpectedStatus(resp.status, req.method, req.uri)) |
There was a problem hiding this comment.
Is this case worth logging a warning that the behavior is buggy?
There was a problem hiding this comment.
Basically, I believe this case shouldn't appear at runtime, because 99% of effect usage is CE.IO/monix.Task/etc effect that is supposed to have a Concurrent instance. But defaultOnError is used for serving requests overall, and end-users could even don't use logging, but this method actually will be used. So this case preserves the current behavior of request processing mostly, not just logging behavior.
|
@armanbilge Is your point strict for merging? Don't you mind if we go ahead with this as is? |
This fixes #3707.
Credit to @ChristopherDavenport and @rossabaker for their work/discussion in #3708.