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

Upgrade to ZIO 2.0.0-RC5 #1194

Merged
merged 2 commits into from
Apr 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion project/Dependencies.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ object Dependencies {
val NettyVersion = "4.1.75.Final"
val NettyIncubatorVersion = "0.0.13.Final"
val ScalaCompactCollectionVersion = "2.7.0"
val ZioVersion = "2.0.0-RC4"
val ZioVersion = "2.0.0-RC5"
val SttpVersion = "3.3.18"

val `jwt-core` = "com.github.jwt-scala" %% "jwt-core" % JwtCoreVersion
Expand Down
10 changes: 5 additions & 5 deletions zio-http/src/main/scala/zhttp/service/ChannelFactory.scala
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ import io.netty.incubator.channel.uring.IOUringSocketChannel
import zio.{UIO, ZIO, ZLayer}

object ChannelFactory {
def nio: ZLayer[Any, Nothing, ChannelFactory] = Live.nio.toLayer
def epoll: ZLayer[Any, Nothing, ChannelFactory] = Live.epoll.toLayer
def uring: ZLayer[Any, Nothing, ChannelFactory] = Live.uring.toLayer
def embedded: ZLayer[Any, Nothing, ChannelFactory] = Live.embedded.toLayer
def auto: ZLayer[Any, Nothing, ChannelFactory] = Live.auto.toLayer
def nio: ZLayer[Any, Nothing, ChannelFactory] = ZLayer(Live.nio)
def epoll: ZLayer[Any, Nothing, ChannelFactory] = ZLayer(Live.epoll)
def uring: ZLayer[Any, Nothing, ChannelFactory] = ZLayer(Live.uring)
def embedded: ZLayer[Any, Nothing, ChannelFactory] = ZLayer(Live.embedded)
def auto: ZLayer[Any, Nothing, ChannelFactory] = ZLayer(Live.auto)

def make[A <: Channel](fn: () => A): UIO[JChannelFactory[A]] = ZIO.succeed(new JChannelFactory[A] {
override def newChannel(): A = fn()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ import zhttp.service.{ChannelFactory, ServerChannelFactory}
import zio.{UIO, ZLayer}

object ServerChannelFactory {
def nio: ZLayer[Any, Nothing, ServerChannelFactory] = Live.nio.toLayer
def epoll: ZLayer[Any, Nothing, ServerChannelFactory] = Live.epoll.toLayer
def uring: ZLayer[Any, Nothing, ServerChannelFactory] = Live.uring.toLayer
def auto: ZLayer[Any, Nothing, ServerChannelFactory] = Live.auto.toLayer
def nio: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.nio)
def epoll: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.epoll)
def uring: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.uring)
def auto: ZLayer[Any, Nothing, ServerChannelFactory] = ZLayer(Live.auto)
Comment on lines +12 to +15
Copy link

@rbobillot rbobillot Apr 11, 2022

Choose a reason for hiding this comment

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

Maybe am I missing something, but shouldn't these be declared as ULayers ? (following the ZIOs declaration logic inside Live)
(same question for ChannelFactory.scala, and the def live: ZLayer[Any, Nothing, DynamicServer] value in DynamicServer.scala)

  ...
  def nio: Ulayer[ServerChannelFactory] = ZLayer(Live.nio)
  ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ULayer[A] is a type alias for ZLayer[Any, Nothing, A]. Which one you use is a matter of preference. Some people like the ULayer type alias because it is more terse. Others like to stick with ZLayer because it is fewer names that people need to remember. I tend to prefer not to use the type aliases for layers but here I was just maintaining the existing style.

Choose a reason for hiding this comment

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

All good for me 😊

Copy link

Choose a reason for hiding this comment

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

Sorry for pestering, any chance we can get this merged in? I'm working on similar PR for softwaremill/tapir and would like to skip the RC6 release of zio-http and go directly to the release that includes this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think we need to figure out what is going on with the test failures here. I'm not quite sure what is going on since all the tests are passing for me locally.

Copy link

@rbobillot rbobillot Apr 12, 2022

Choose a reason for hiding this comment

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

The tests fail also locally for me.
Something gets slow within the websocket upgrades: (https://github.com/dream11/zio-http/blob/main/zio-http/src/test/scala/zhttp/service/WebSocketServerSpec.scala)

  def websocketServerSpec = suite("WebSocketServer") {
    suite("connections") {
      testM("Multiple websocket upgrades") {
        val app   = Socket.succeed(WebSocketFrame.text("BAR")).toHttp.deployWS
        val codes = ZIO
          .foreach(1 to 1024)(_ => app(Socket.empty.toSocketApp).map(_.status)) // here
          .map(_.count(_ == Status.SwitchingProtocols))

        assertM(codes)(equalTo(1024))
      }
    }
  }

Of course, setting the timeout to 30 seconds makes it work,
so does a foreachPar rather than a foreach, but neither of these would be viable solutions (imo).
For now, I still can't get what's going wrong :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, that's really interesting. Those tests run in a couple seconds for me locally.

Copy link

@rbobillot rbobillot Apr 14, 2022

Choose a reason for hiding this comment

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

Ok, I guess we might run towards a solution here !

I ran tests, with multiple Scala versions (2.12, 2.13, 3.1), on a few computers, and there's only one where the tests fail:

  • Xiaomi Notebook Pro: 🟢 (CPU: Intel i7 (8 cores) @ 4.900GHz, Memory: 16Go DDR4 3200MT/s)
  • Thinkpad P15v: 🟢 (CPU: Intel i7 (16 cores) @ 5.100GHz, Memory: 64Go 2667MT/s)
  • Macbook Air: 🔴 (CPU: Intel i7 (2 cores) @ 1.700GHz, Memory: 8Go DDR3 1600MHz)

Updating available memory on the MacBook (via SBT_OPTS), didn't change anything
Hense, the CPU speed, might be responsible for the slow Websockets bulk spawning/updating.

However, it's quite something that this slow down only appears now (since RC5)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, well increasing the timeout seems to have have at least allowed it to pass CI. Do you have a sense for the relative magnitude of the change? I am surprised by it as well and we can definitely follow up.


object Live {
def nio: UIO[JChannelFactory[ServerChannel]] = ChannelFactory.make(() => new NioServerSocketChannel())
Expand Down
2 changes: 1 addition & 1 deletion zio-http/src/test/scala/zhttp/http/HExitSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import zio.test._
import zio.{ZIO, durationInt}

object HExitSpec extends ZIOSpecDefault with HExitAssertion {
def spec: ZSpec[Environment, Failure] = {
def spec: ZSpec[Environment, Any] = {
import HExit._
suite("HExit")(
test("out") {
Expand Down
13 changes: 7 additions & 6 deletions zio-http/src/test/scala/zhttp/internal/DynamicServer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@ object DynamicServer {

def httpURL: ZIO[DynamicServer, Nothing, String] = baseURL(Scheme.HTTP)

def live: ZLayer[Any, Nothing, DynamicServer] = {
for {
ref <- Ref.make(Map.empty[Id, HttpApp[Any, Throwable]])
pr <- Promise.make[Nothing, Start]
} yield new Live(ref, pr)
}.toLayer
def live: ZLayer[Any, Nothing, DynamicServer] =
ZLayer {
for {
ref <- Ref.make(Map.empty[Id, HttpApp[Any, Throwable]])
pr <- Promise.make[Nothing, Start]
} yield new Live(ref, pr)
}

def port: ZIO[DynamicServer, Nothing, Int] = ZIO.environmentWithZIO[DynamicServer](_.get.port)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ object WebSocketServerSpec extends HttpRunnableSpec {

override def spec = suite("Server") {
app.as(List(websocketServerSpec, websocketFrameSpec))
}.provideLayerShared(env) @@ timeout(10 seconds)
}.provideLayerShared(env) @@ timeout(60 seconds) @@ zio.test.TestAspect.withLiveClock

def websocketServerSpec = suite("WebSocketServer") {
suite("connections") {
Expand Down