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

Port servlet to cats-effect-3 #4175

Merged
merged 1 commit into from
Jan 12, 2021
Merged

Port servlet to cats-effect-3 #4175

merged 1 commit into from
Jan 12, 2021

Conversation

ashwinbhaskar
Copy link
Collaborator

No description provided.

@ashwinbhaskar ashwinbhaskar changed the title Fixes #4091 WIP Fixes #4091 Jan 10, 2021
@ashwinbhaskar
Copy link
Collaborator Author

@rossabaker @yanns this is still WIP. But I wanted to know if I am on the right track. I have ported 3 files to ce3 - AsyncHttp4sServlet, BlockingHttp4sServlet and Http4sServlet. Would request you to review these 3 files especially the Blocker changes.

Comment on lines +110 to +116
val result = F
.attempt(f)
.flatMap {
case Right(()) => F.unit
case Left(e) => F.delay(logger.error(e)("Error in error handler"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should work no?

Suggested change
val result = F
.attempt(f)
.flatMap {
case Right(()) => F.unit
case Left(e) => F.delay(logger.error(e)("Error in error handler"))
}
val result = F
.attempt(f)
.flatMap(loggingAsyncCallback(logger))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yanns loggingAsyncCallback requires an implicit Sync. But we have an implicit Async with us.

Copy link
Contributor

Choose a reason for hiding this comment

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

trait Async[F[_]] extends AsyncPlatform[F] with Sync[F] with Temporal[F]

So it should work no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@yanns I get this error

n/scala/org/http4s/servlet/AsyncHttp4sServlet.scala:69:38: could not find implicit value for parameter F: cats.effect.Sync[F] (Could not find an instance of Monad for F)
[error]         .flatMap(loggingAsyncCallback(logger))

Copy link
Contributor

Choose a reason for hiding this comment

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

OK thanks for trying.
I don't really understand why, but I'll check later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It was changed by 4c5ab30

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you should rebase your change on the newest version of main?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, you were right. I had to rebae with main:) But still this error persists

n/scala/org/http4s/servlet/AsyncHttp4sServlet.scala:69:38: could not find implicit value for parameter F: cats.effect.Sync[F] (Could not find an instance of Monad for F)
[error]         .flatMap(loggingAsyncCallback(logger))

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks for thaving tried!

@ashwinbhaskar ashwinbhaskar changed the title WIP Fixes #4091 Fixes #4091 Jan 10, 2021
onParseFailure(_, servletResponse, bodyWriter),
handleRequest(ctx, _, bodyWriter)
))
.flatMap {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking about whether we want a guarantee here. What should this do in case of cancellation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rossabaker how do we cancel the request?

Copy link
Member

Choose a reason for hiding this comment

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

Losing the race in handleRequest would cancel it. I'll have to think some more about what we should do about that. If it passes the tests... I'm good with it for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests of servlet project pass for now

val HttpSession: Key[Option[HttpSession]] = {
val result = Key.newKey[F, Option[HttpSession]]
dispatcher.unsafeRunSync(result)
}
Copy link
Member

Choose a reason for hiding this comment

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

Vault on CE3 should perhaps have a Key.unsafeNewKey[F, A](dispatcher)? /cc @ChristopherDavenport

case Blocked(cb) => read(cb)
case _ => ()
}
F.async_[Option[Chunk[Byte]]] { cb =>
Copy link
Member

Choose a reason for hiding this comment

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

Does this also take care of shifting?

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar Jan 11, 2021

Choose a reason for hiding this comment

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

@rossabaker From what I understood by reading this, shifting happens automatically. Would be great if you could confirm that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to recall why that had to trampoline before. I think evalOn would run the effect on a specified EC and then shift back. This shifted once and never back, which I don't see a way to do without ContextShift.

I'm content to try without this and see if it works.

@rossabaker rossabaker changed the title Fixes #4091 Port servlet to cats-effect-3 Jan 12, 2021
@rossabaker rossabaker merged commit d48b9ed into http4s:main Jan 12, 2021
rossabaker added a commit to http4s/http4s-servlet that referenced this pull request Apr 3, 2022
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.

3 participants