migrate json4s, json4s-native, json4s-jackson to cats-effect 3 #3885
Conversation
build.sbt
Outdated
| @@ -33,7 +33,7 @@ lazy val modules: List[ProjectReference] = List( | |||
| // boopickle, | |||
| // circe, | |||
| // json4s, | |||
There was a problem hiding this comment.
You'll want to uncomment this line, too. You're already tackling this common module.
| F.pure( | ||
| Try(reader.read(json)).toEither | ||
| .leftMap[DecodeFailure](e => InvalidMessageBodyFailure("Could not map JSON", Some(e))) | ||
| )) |
There was a problem hiding this comment.
Yes, F.pure is correct here. I think the old delay was capturing exceptions.
This could be microoptimized and avoid the allocation of a Try with a regular old try/catch expression.
There was a problem hiding this comment.
Thx for the feedback.
I was not sure if a good old try / catch block was an ok-style in https.
I'll change that.
|
I wouldn't flag things that take CPU time. We want this executing on one of the CPU bound pools, sized to one per core. It's blocking IO that we want to shift to blocking pools, where it's more typical to use a cached (or at least larger) thread pool for things not contending for the CPU. |
|
I've also activated |
There was a problem hiding this comment.
I don't think try/catch is bad when wrapping a library that fails by throwing. I think it's bad when we fail by throwing.
If you're just wrapping success in Right and failure in Left, there is Either.catchNonFatal with import cats.implicits._ that would hide the try and catch. You could use that here, but then you'd need to leftMap it. I think what you've done is good, with a small tweak below.
| F.pure { | ||
| try Right(reader.read(json)) | ||
| catch { | ||
| case e: Exception => Left(InvalidMessageBodyFailure("Could not map JSON", Some(e))) |
There was a problem hiding this comment.
I'd go with NonFatal(e) instead of e: Exception here.
rossabaker
changed the title
I'd like to enable some tests to check #3871
Now that we are using
Concurrentinstead ofSync, we cannot useF.delayanymore.I'm not sure that would be the right approach here.
I've started with
pure.As the parsing of a json can take CPU time, should we signal it that some way so that maybe a new thread should be used for it? Could this block a fiber?
Should we use something different from
F.pure()?F.unit.as()?