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

Add BlockingHttp4sServlet #1830

Merged
merged 3 commits into from May 9, 2018
Merged

Add BlockingHttp4sServlet #1830

merged 3 commits into from May 9, 2018

Conversation

@Taig
Copy link
Contributor

@Taig Taig commented May 7, 2018

The current Http4sServlet uses the async Servlet feature in its implementation. Unfortunately this is not supported in the Google App Engine (GAE) standard environment so that a blocking implementation is necessary.

Consider this PR as an initial draft. I started off by copying the Http4sServlet and ripping it apart. So there is currently quite a lot of code duplication which should be improved.

Please note that I have never worked with Servlet nor with cats.Effect before, so unfortunately some of the changes here were a bit of guesswork for me.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks!

This is going to be unpopular, but if we move most of this into an abstract class, and leave the service method abstract, we could extend that with a small amount of code in both BlockingServlet and Http4sServlet. I prefer to avoid OO, but if we're not going to do it when implementing JEE, then where?

Alternatively, we could pass an (HttpServletRequest, HttpServletResponse) => Unit as a constructor param, but that has weird interactions with the timeout and servletIo parameters.


class BlockingHttp4sServlet[F[_]](
service: HttpRoutes[F],
servletIo: BlockingServletIo[F],

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

What's the benefit of parameterizing this? To configure the chunk size? I like that it mandates BlockingServletIo for this use case.

This comment has been minimized.

@Taig

Taig May 8, 2018
Author Contributor

I wanted to keep the chunkSize configurable but also mimic the API of Http4sSevlet for consistency

)

F.runAsync(render) {
case Right(_) => Sync[IO].unit

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

IO.unit


F.runAsync(render) {
case Right(_) => Sync[IO].unit
case Left(t) => Sync[IO].delay(errorHandler(servletResponse)(t))

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

IO(errorHandler(...))

.unsafeRunSync()
} catch errorHandler(servletResponse)

private def onParseFailure(

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

I think everything here below is a copy-paste and could move into an abstract class? Make them protected.

extends HttpServlet {
private[this] val logger = getLogger

private[this] var serverSoftware: ServerSoftware = _

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

Could be protected and moved to abstract class.

private[this] var serverSoftware: ServerSoftware = _

// micro-optimization: unwrap the service and call its .run directly
private[this] val serviceFn = service.run

This comment has been minimized.

@rossabaker

rossabaker May 7, 2018
Member

Could be protected and move to abstract class. Though maybe we save a few nanoseconds going through a field (e.g., private[this]) instead of an accessor.

@Taig
Copy link
Contributor Author

@Taig Taig commented May 8, 2018

Any naming preferences for the abstract class? I would probably name it Http4sServlet and extend from AsyncHttp4sServlet and BlockingHttp4sServlet. But I assume we should not rename the current Http4sServlet for compatibility?

@Taig Taig force-pushed the Taig:develop branch from f72899b to e5debd7 May 8, 2018
Copy link
Member

@rossabaker rossabaker left a comment

I think I can live with the breaking change on master, but that means we can't add it in the 0.18 series, and you probably want this now.

If we call the base class HttpServletLike (ugh), we could introduce BlockingHttpServlet and probably keep binary compatibility with Http4sServlet and release it in 0.18.11.

The division looks good.

@Taig
Copy link
Contributor Author

@Taig Taig commented May 8, 2018

I'm fine with keeping my own copy of this around until 0.19 is released, so that's not an issue for me. Let me know if you want the renaming, I don't mind waiting for 0.19.

Taig added 3 commits May 7, 2018
@Taig Taig force-pushed the Taig:develop branch from e5debd7 to b76ad53 May 8, 2018
Copy link
Member

@rossabaker rossabaker left a comment

We could even backport the copy-and-pasted version to 0.18 if you need it now. I think it's better to move forward with the better names, so this looks good for master.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

If we're happy waiting, I'm happy with it. 👍

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 9, 2018

Test failure is related, fixed by #1836.

@rossabaker rossabaker merged commit 37bf4e7 into http4s:master May 9, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@Taig Taig deleted the Taig:develop branch May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants