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 a client backend for HttpURLConnection #1882

Merged

Conversation

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 28, 2018

It's not production grade. It blocks. But it comes with no extra dependencies, doesn't need to be shut down, and is nice for a REPL session.

object HttpURLConnectionClient {

/** Construct a new Client */
def apply[F[_]](implicit F: Effect[F]): Client[F] =

This comment has been minimized.

@rossabaker

rossabaker May 28, 2018
Author Member

We probably want a configuration here for timeouts?

@rossabaker rossabaker force-pushed the rossabaker:http-url-connection-client branch from f3906bc to b63346b Jun 17, 2018
import scala.concurrent.ExecutionContext

package object internal {
def blocking[F[_], A](fa: F[A], blockingExecutionContext: ExecutionContext)(implicit F: Async[F], ec: ExecutionContext): F[A] =

This comment has been minimized.

@rossabaker

rossabaker Jun 17, 2018
Author Member

This looks a lot like @ChristopherDavenport's linebacker 0.1 and the not ideal combinator in @SystemFw's gist.

Better can be done with bracket. Better still can be done with rank-N types. But maybe this is the best we can do now?

val hostnameVerifier: Option[HostnameVerifier],
val sslSocketFactory: Option[SSLSocketFactory],
val blockingExecutionContext: ExecutionContext,
)(implicit ec: ExecutionContext) {

This comment has been minimized.

@rossabaker

rossabaker Jun 17, 2018
Author Member

This is a good example of what I have in mind for the client and server configurations. This can be evolved by adding more values. Generating the with methods is tedious, but I think compatibility wins here.

This comment has been minimized.

@rossabaker

rossabaker Jun 17, 2018
Author Member

Maybe having types named BlahClient create a plain old Client is confusing. We could all this a JavaNetClientBuilder or JavaNetClientConfig, but I like the shape.

Copy link
Member

@SystemFw SystemFw left a comment

Quite useful in the REPL, yeah.
Eventually blocking should be in cats-effect and using bracket (for posterity, you already know this :) )

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jun 18, 2018

It's still not great. The body I/O blocks. readInputStreamAsync and writeOutputStreamAsync shift, but don't shift back. We could make a double shifting version of those, but not without copying in parts of JavaInputOutputStream.

@rossabaker rossabaker dismissed SystemFw’s stale review Jun 18, 2018

Added double shift for bodies

@@ -0,0 +1,39 @@
http4s
Copyright 2013-2018 http4s.org
Licensed under Apache License 2.0 (see LICENSE)

This comment has been minimized.

@rossabaker

rossabaker Jun 18, 2018
Author Member

We're technically supposed to include the license of all the projects we copied from. I took the liberty of doing so while adding yet another project to copy and paste from.

A former employer's lawyer once told me we need to have the copyright boilerplate on all our source files. I told him not while we were maintaining multiple branches. But maybe now is a good time. But that's a discussion for another ticket.

fa0 <- F.fromEither(att)
} yield fa0

/** Like fs2.io.readInputStream, but does a double shift with [[blocking]]. */

This comment has been minimized.

@rossabaker

rossabaker Jun 18, 2018
Author Member

I think everything from here below disappears in fs2-1.0, because it does a double shift. This is basically all a backport to fs2-0.10.

Licensed under Apache License 2.0 (see LICENSE)

This software contains portions of code derived from finagle
https://github.com/twitter/finagle

This comment has been minimized.

@rossabaker

rossabaker Jun 18, 2018
Author Member

The version we copied from has no copyright. 😕

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jun 26, 2018

Bump. I dismissed @SystemFw's earlier approval due to substantial changes.

@rossabaker rossabaker requested review from SystemFw and aeons Jul 3, 2018
@aeons
aeons approved these changes Jul 3, 2018
Copy link
Member

@aeons aeons left a comment

We can't really get around this blocking somewhere, so I think packing it away like this is a good solution.

@SystemFw
Copy link
Member

@SystemFw SystemFw commented Jul 3, 2018

It's a bit of a shame since the main use for this is probably the repl, where you normally just import global, but it is what it is

@rossabaker
Copy link
Member Author

@rossabaker rossabaker commented Jul 3, 2018

Yeah, it made it slightly more awkward to use in the REPL, but it'll be a bit more consistent with our other java.io builders.

You know, we could have a repl constructor for this one that just blocks the current thread because lol nothing matters.

@rossabaker rossabaker merged commit ed19bf9 into http4s:release-0.18.x Jul 3, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rossabaker rossabaker deleted the rossabaker:http-url-connection-client branch Jul 3, 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

4 participants