-
Notifications
You must be signed in to change notification settings - Fork 800
Fixes #4084 Port jetty to cats effect 3 #4191
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
Conversation
@rossabaker The tests in JettyServerSuite all time out after 30 seconds and result in error. Where did I go wrong in that? |
import org.eclipse.jetty.server.{HttpConfiguration, HttpConnectionFactory, Server, ServerConnector} | ||
import org.eclipse.jetty.servlet.{ServletContextHandler, ServletHolder} | ||
import org.http4s.dsl.io._ | ||
import org.http4s.servlet.AsyncHttp4sServlet | ||
import org.http4s.syntax.all._ | ||
|
||
object Issue454 { | ||
implicit val cs: ContextShift[IO] = Http4sSpec.TestContextShift | ||
// implicit val cs: ContextShift[IO] = Http4sSpec.TestContextShift |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code doesn't ever run as part of the build. If we can't figure out a way to automate it, then I think it's ready to be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it
jetty/src/test/scala/org/http4s/server/jetty/JettyServerSuite.scala
Outdated
Show resolved
Hide resolved
jetty/src/test/scala/org/http4s/server/jetty/JettyServerSuite.scala
Outdated
Show resolved
Hide resolved
@@ -224,7 +233,8 @@ sealed class JettyBuilder[F[_]] private ( | |||
service = service, | |||
asyncTimeout = builder.asyncTimeout, | |||
servletIo = builder.servletIo, | |||
serviceErrorHandler = builder.serviceErrorHandler | |||
serviceErrorHandler = builder.serviceErrorHandler, | |||
dispatcher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some other backends are creating dispatcher
in Resource
, which is a pattern I like. The challenge would be how you can thread it down to here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you suggesting to pass Resource[IO,Dispatcher[IO]]
instead of Dispatcher[IO]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'm saying to let one of the functions in this create it. In BlazeClientBuilder (yes, that's a client, and this is a server, but... almost the same), we're creating and using a dispatcher
resource in the .resource
function. Can we do that here?
I think if the function in the Mount
class took a Dispatcher
, the rest of it would fall into place, and you could remove the Dispatcher
as a parameter of the builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, let me try that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rossabaker the resource method returns a Resource
with type parameter F
. That means we should have a Dispatcher[F]
in scope. We can change the signature of def resource: Resource[F, Server]
to def resource(dispatcher: Dispatcher[F]): Resource[F, Server]
. But this would mean changing the signature on the trait ServerBuilder
.
If we want to avoid this, then we would have to pass a dispatcher to the builder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I was thinking that resource would create a Dispatcher
resource and flatMap it. BlazeClientBuilder
does it like this:
def resource: Resource[F, Client[F]] =
for {
dispatcher <- Dispatcher[F]
...
} yield ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rossabaker my bad. You are right. Having an implicit Async[F]
brings Dispatcher[F]
in scope
Are we past the timeout problem now? |
@rossabaker no, the timeout issue still persists. |
Oh, the module just isn't enabled yet. I didn't see the failing test and was hoping we were through it. Do you have any ideas yet or do you need some help? |
I am still trying to figure out the issue. Any help is welcome:) |
Oh, I thought we were deleting Issue454 entirely since it isn't a test that gets run. |
okay, I understood it as deleting the implicit line 😅 |
Added some log statements. It confirms this line is causing the timeout. |
Try |
results in |
@rossabaker The issue was in servlet module. |
@@ -82,7 +82,8 @@ class AsyncHttp4sServlet[F[_]]( | |||
|
|||
val timeout = | |||
F.async_[Response[F]] { cb => | |||
val _ = gate.complete(ctx.addListener(new AsyncTimeoutHandler(cb))) | |||
val _ = | |||
dispatcher.unsafeRunSync(gate.complete(ctx.addListener(new AsyncTimeoutHandler(cb)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to block here you should be able to just do
F.async[Response[F]] { cb =>
gate.complete(ctx.addListener(new AsyncTimeoutHandler(cb))).as(Option.empty[F[Unit]])
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Fixes http4s/http4s#4084 Port jetty to cats effect 3
Fixes http4s/http4s#4084 Port jetty to cats effect 3
No description provided.