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

Websocket support for handhake headers (such as:Sec-WebSocket-Protoco… #1607

Merged
merged 10 commits into from Dec 22, 2017

Conversation

@jodagm
Copy link
Contributor

@jodagm jodagm commented Dec 20, 2017

@rossabaker following your guidance at http4s/http4s-websocket#7
I started with a minimal approach in the spirit of the existing WS API (thinking about current usage of WS), so I just added another overload with the additional headers. I moved WebSocketContext to the core alongside Websocket.

I could not make the compiler happy with the default responses as def, I would need your help to appease the implicit Gods :)

Copy link
Member

@rossabaker rossabaker left a comment

Thanks! This looks good. A couple small suggestions.

@@ -52,14 +52,19 @@ private[blaze] trait WebSocketSupport[F[_]] extends Http1ServerStage[F] {
hdrs.foreach {
case (k, v) => sb.append(k).append(": ").append(v).append('\r').append('\n')
}

val wsContext = ws.get

This comment has been minimized.

@rossabaker

rossabaker Dec 20, 2017
Member

This is not your fault, but it's something we can clean up. Can we do a match on the ws Option instead of if (ws.isDefined) { ???; ws.get ; ??? } else { ??? }?

This comment has been minimized.

@jodagm

jodagm Dec 20, 2017
Author Contributor

sure, I'll change that

*/
def WS[F[_]](

This comment has been minimized.

@rossabaker

rossabaker Dec 20, 2017
Member

WS is kind of a strange name. I think we could deprecate the old WS constructors and make the one we really want here. I still like:

def WebSocketResponse(
    send: Stream[F, WebSocketFrame],
    receive: Sink[F, WebSocketFrame],
    headers: Headers = Headers.empty, // Sec-WebSocket-Protocol, Cookies, etc. go here
    onNonWebSocketRequest: F[Response[F]] = DefaultNonWebSocketResponse,
    onHandshakeFailure: F[Response[F]] = DefaultHandshakeFailureResponse)

That arranges the default arguments in the order they're most likely to be overridden. If you want to set aside the custom handshake failure for now, you can just leave that off the end (as well as the context, which you've already done).

This comment has been minimized.

@jodagm

jodagm Dec 20, 2017
Author Contributor

I need your help with the proper definition of DefaultNonWebSocketResponse, tried to do it with def so I can specify the F as its defined on def WS, but I think I got the implicit wrong.

This comment has been minimized.

@jodagm

jodagm Dec 20, 2017
Author Contributor

I think I should be doing

private def DefaultNonWebSocketResponse[F[_]](implicit F: Monad[F]) = Response[F](Status.NotImplemented).withBody("This is a WebSocket route.")

This comment has been minimized.

@rossabaker

rossabaker Dec 20, 2017
Member

That looks right. You could alternately use the shorthand:

private def DefaultNonWebSocketResponse[F: Monad] =

Your syntax gives you access to F if you want to invoke methods directly on the type class. The shorthand just guarantees that F has a Monad. I usually use the shorthand unless I need the named version in the definition.

This comment has been minimized.

@jodagm

jodagm Dec 20, 2017
Author Contributor

my issue then is with the using it as default param value:

onNonWebSocketRequest: F[Response[F]] = DefaultNonWebSocketResponse

getting

Error:(59, 66) ambiguous implicit values:
 both value catsInstancesForId in package cats of type => cats.Bimonad[cats.package.Id] with cats.CommutativeMonad[cats.package.Id] with cats.Comonad[cats.package.Id] with cats.NonEmptyTraverse[cats.package.Id]
 and method catsStdInstancesForEither in trait EitherInstances of type [A]=> cats.MonadError[[β$0$]scala.util.Either[A,β$0$],A] with cats.Traverse[[β$1$]scala.util.Either[A,β$1$]]
 match expected type cats.Monad[F]
                         onNonWebSocketRequest: F[Response[F]] = DefaultNonWebSocketResponse,

This comment has been minimized.

@SystemFw

SystemFw Dec 21, 2017
Member

This is a minimisation of the problem, let's assume:

  import cats._, implicits._
  def default[F[_]: Monad]: F[Int] = 1.pure[F]

then:

  case class No[F[_]: Monad](s: String, f: F[Int] = default[F])

Does not compile. You can do:

  case class Yes[F[_]](s: String, f: F[Int])
  object Yes {
    class Builder[F[_]: Monad] {
      def apply(s: String, f: F[Int] = default[F]) = Yes(s, f)
    }

    def apply[F[_]: Monad] = new Builder[F]
  }

  def yes = Yes[Option].apply("yes") // Yes(yes, Some(1))

Ideally, we should be able to call yes with Yes[Option]("yes"), but when we do that the compiler thinks we are passing evidence for Monad explicitly, and complains. This is annoying, and there's a proposal to fix it in the language (the explicitly keyword). (Note that we should also transform Yes to allow for an overriden apply like I do below, but the implicit thing is a showstopper anyway)

There is a solution if you're willing to crank up the magic though...

  // you lose apply, which you want to lose since it's overriden
  // you lose copy, which in this case  kinda sucks
  // you still have equals, hash and toString
  sealed abstract case class Magic[F[_]](s: String, f: F[Int])
  object Magic {
    // same builder as before
    class Builder[F[_]: Monad] {
      def apply(s: String, f: F[Int] = default[F]) = new Magic(s, f){}
    }
    // magic
    class Carrier[F[_]]
    object Carrier {
      implicit def conv[F[_]](c: Carrier[F])(implicit F: Monad[F]): Builder[F] =
        new Builder[F]
    }

    def apply[F[_]] = new Carrier[F]
  }

  def magic = Magic[Option]("magic") // Magic(magic, Some(1))

This comment has been minimized.

@aeons

aeons Dec 21, 2017
Member

Yeah, I was a bit unclear in my explanation, You can do what @SystemFw suggests or see how it's done for BlazeBuilder here: https://github.com/http4s/http4s/blob/master/blaze-server/src/main/scala/org/http4s/server/blaze/BlazeBuilder.scala#L294

So the apply on the companion object only takes the implicit evidence, and then you use the .with* methods on the builder to change the things you want to change from the defaults.

This comment has been minimized.

@SystemFw

SystemFw Dec 21, 2017
Member

fwiw, I think the best tradeoff between convenience and clarity is method no.2, except you rename apply to build, and use named parameters rather than multiple with* methods.
Method no.3 is interesting enough to be worth experimenting with imho, but it's a bit of a tradeoff (error messages, weird encoding)

This comment has been minimized.

@aeons

aeons Dec 22, 2017
Member

I tried using the partially applied approach on BlazeBuilder, but it fails if you don't set any of the arguments.

BlazeBuilder.build[IO] fails, BlazeBuilder.build[IO]() fails since it expects the Effect evidence there.

With the carrier, you still have to give it empty parens to make it work, which imo is worse ergonomics than what we have currently, which works with BlazeBuilder[IO].with...

This comment has been minimized.

@SystemFw

SystemFw Dec 22, 2017
Member

yeah you need BlazeBuilder[IO].build(). The parenthesis are unavoidable due to the way default arguments work in Scala (for some mysterious reason), it probably works best in cases where there's at least one argument that does not have a default

Response[F](Status.NotImplemented).withBody("This is a WebSocket route.")
private var onHandshakeFailure: F[Response[F]] =
Response[F](Status.BadRequest).withBody("WebSocket handshake failed.")
private var handshakeResponseHeaders: Headers = Headers.empty

This comment has been minimized.

This comment has been minimized.

@jodagm

jodagm Dec 22, 2017
Author Contributor

@rossabaker @aeons @SystemFw thank you for your feedback, I am learning a lot from you guys. I used BlazeBuilder as reference. Still wrapping my head around the (dark) Magic example, but as its now 02:00 for me, I will sure have to return to this tomorrow morning. I hope the current state of the code is good enough to be approved, though.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Dec 21, 2017

What does everybody think about the builder approach vs. the carrier solution? The carrier is a cleaner call when done right, but more complicated machinery if the call is made wrong.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Dec 21, 2017

Oops, I had missed the further conversation in my previous comment.

Copy link
Member

@rossabaker rossabaker left a comment

I think we should go ahead and deprecate the WS methods.

I think this code looks good. I think the open question is whether we prefer the builder or the partially applied techique. What's here:

WebSocketBuilder(send, recv).toHttpResponse

vs. @SystemFw's idea:

WebSocketBuilder.build(send, recv)
@aeons
Copy link
Member

@aeons aeons commented Dec 22, 2017

I like the .build version.


@deprecated("Use WebSocketBuilder", "0.18")

This comment has been minimized.

@rsoeldner

rsoeldner Dec 22, 2017
Contributor

Shouldn't the version be "0.18.0-M7" ?

This comment has been minimized.

This comment has been minimized.

@jodagm

jodagm Dec 22, 2017
Author Contributor

fixing

Copy link
Member

@rossabaker rossabaker left a comment

👍

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

LGTM 👍

@rossabaker rossabaker merged commit 820e0fe into http4s:master Dec 22, 2017
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
@jodagm jodagm deleted the jodagm:websocket branch Dec 23, 2017
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

6 participants
You can’t perform that action at this time.