Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to cats-effect 3 & http4s 0.23 #1154

Merged
merged 49 commits into from
Jul 15, 2021
Merged

Migrate to cats-effect 3 & http4s 0.23 #1154

merged 49 commits into from
Jul 15, 2021

Conversation

adamw
Copy link
Member

@adamw adamw commented Apr 9, 2021

See: #1050

case None => headers
}
Ok(headers = filteredHeaders: _*)
okOnlyHeaders(filteredHeaders.map(x => x: Header.ToRaw))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catostrophe if you'd have a better idea how to handle this case I'd be happy to take hints :) Here I've get a list of raw headers and I want to send them back (and only the headers). I couldn't get it to work without the (otherwise useless) map with a cast (triggering the implicit conversion), but it's probably because of friday evening not because it's necessary ;)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    case r @ GET -> Root / "api" / "echo" / "headers" =>
      val headers = r.headers.headers.map(h => h.copy(value = h.value.reverse))
      val filteredHeaders: Header.ToRaw = r.headers.headers.find(_.name == CIString("Cookie")) match {
        case Some(c) => headers.filter(_.name == CIString("Cookie")) :+ Header.Raw(CIString("Set-Cookie"), c.value.reverse)
        case None    => headers
      }
      okOnlyHeaders(List(filteredHeaders))
    case r @ GET -> Root / "api" / "echo" / "param-to-header" =>
      okOnlyHeaders(r.uri.multiParams.getOrElse("qq", Nil).reverse.map("hh" -> _: Header.ToRaw))
    case r @ GET -> Root / "api" / "echo" / "param-to-upper-header" =>
      okOnlyHeaders(r.uri.multiParams.map { case (k, v) =>
        k -> v.headOption.getOrElse("?"): Header.ToRaw
      }.toSeq)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would write it this way

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, much better :)

response: ServerResponse[Http4sResponseBody[F]]
): F[Response[F]] = {
val statusCode = statusCodeToHttp4sStatus(response.code)
val headers = Headers(response.headers.map { case SttpHeader(k, v) => Header.Raw(CaseInsensitiveString(k), v) }.toList)
val headers = Headers(response.headers.map { case SttpHeader(k, v) => Header.Raw(CIString(k), v) }.toList)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val headers = Headers(response.headers.map(h => h.name -> h.value))

@adamw
Copy link
Member Author

adamw commented Apr 12, 2021

I've got a problem converting the finatra-cats interpreter. It has the following:

FinatraServerInterpreter.toRoute(e.endpoint.serverLogic(i => eff.toIO(e.logic(new CatsMonadError)(i)).to[Rerunnable].run))

specifically, there's eff.toIO which enables to run the effect to a future. Is there something similar in ce3? That is, a way to run an arbitrary Async effect? Any typeclass to require?

@catostrophe
Copy link

I've got a problem converting the finatra-cats interpreter. It has the following:

FinatraServerInterpreter.toRoute(e.endpoint.serverLogic(i => eff.toIO(e.logic(new CatsMonadError)(i)).to[Rerunnable].run))

specifically, there's eff.toIO which enables to run the effect to a future. Is there something similar in ce3? That is, a way to run an arbitrary Async effect? Any typeclass to require?

@adamw I see. I will provide a solution

@andreamarcolin
Copy link

Is there something similar in ce3? That is, a way to run an arbitrary Async effect? Any typeclass to require?

@adamw I guess Dispatcher could be what you need here? https://typelevel.org/cats-effect/docs/std/dispatcher

@adamw
Copy link
Member Author

adamw commented Apr 12, 2021

@andreamarcolin Ah exactly, thanks! :)

@catostrophe
Copy link

catostrophe commented Apr 12, 2021

@adamw things you should remember:

  • Dispatcher is a resource. When you release it, it cancels all the spawned fibers (futures)
  • you shouldn't pass it implicitly (it is not encouraged by the ce3 authors)
  • you probably need to create one at first, and then create all the finatra routes within its use scope, i.e. it's a global resource with a wide scope, like the http server itself
  • once you enter the use scope, the current runtime context is captured by the dispatcher, and all the further evalOns don't affect the ExecutionContext of the tasks run by the dispatcher

@catostrophe
Copy link

You also need a proper conversion to twitter's Future. catbird still doesn't have a ce3 compatible release

@catostrophe
Copy link

catbird is a blocker for this PR.
see: typelevel/catbird#267 - WIP for CE3-compatible release

# Conflicts:
#	project/Versions.scala
#	server/finatra-server/src/test/scala/sttp/tapir/server/finatra/FinatraTestServerInterpreter.scala
#	server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sRequestBody.scala
#	server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sServerInterpreter.scala
#	server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sServerOptions.scala
#	server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sToResponseBody.scala
#	server/tests/src/main/scala/sttp/tapir/server/tests/TestServerInterpreter.scala
@kubinio123 kubinio123 self-assigned this Apr 30, 2021
@kubinio123 kubinio123 linked an issue May 5, 2021 that may be closed by this pull request
@adamw
Copy link
Member Author

adamw commented May 5, 2021

@catostrophe thanks for the pointers. I think @kubinio123 migrated the code as far as possible, with the exception of the finatra-cats module.

kubinio123 added 2 commits May 5, 2021 16:40
# Conflicts:
#	server/vertx/src/test/scala/sttp/tapir/server/vertx/streams/Fs2StreamTest.scala
@brbrown25
Copy link

@adamw I wanted to check in and see what the status of this pr is? Based on the comments it looks like this might be held up by catbird?

@adamw
Copy link
Member Author

adamw commented May 10, 2021

@brbrown25 yes, that's the only missing component. Another is the fact that we'd have to switch from "stable" (as long as you can call 0.21 stable) http4s releases to 1.x milestone releases. My current plan (which isn't set in stone) is to release tapir 0.18 using http4s 0.21, and then switch to http4s 1.x.

@brbrown25
Copy link

@adamw thanks for the quick reply! that sounds like a reasonable plan, I'll keep an eye on this one. have a great day and thanks for some awesome repositories!

@kubukoz
Copy link
Contributor

kubukoz commented Jul 14, 2021

Small note, PR title should say 0.23 not 1.0 :)

@adamw adamw changed the title Migrate to cats-effect 3 & http4s 1.0 Migrate to cats-effect 3 & http4s 0.23 Jul 14, 2021
@adamw
Copy link
Member Author

adamw commented Jul 14, 2021

@kubukoz true, fixed :)

now the tests which started taking 3h instead of 40m ;)

@calvinlfer
Copy link
Contributor

now the tests which started taking 3h instead of 40m ;)

🤣🙀

@adamw
Copy link
Member Author

adamw commented Jul 14, 2021

Yeah I have yet no idea what's going on, only a slight lead that there's some weird interaction between ce3 and netty. In tapir, the modules which take the longest to test (after the upgrade) are vertx and ziohttp, both of which are netty-based; while the test framework uses ce3. Similarly, in sttp-client, async-http-client backends in the fs2 flavor sometimes take a long time to test (and sometimes not). Maybe we're running something on the wrong thread, and hence the problems, but I didn't find anything specific yet :)

@adamw
Copy link
Member Author

adamw commented Jul 15, 2021

Luckily changing the http client to run the tests from ahc-fs2 to httpclient-fs2 solved, or rather minimized, the problem. The issue still exists in sttp-client, and the vertx tests in tapir are still suspiciously slow. But at least the build passes, so we can proceed.

@adamw adamw merged commit e61458c into master Jul 15, 2021
@mergify mergify bot deleted the cats-effect-3 branch July 15, 2021 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to cats effect 3
10 participants