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

Client cleanup is slow #2403

Closed
johnhungerford opened this issue Aug 26, 2023 · 8 comments
Closed

Client cleanup is slow #2403

johnhungerford opened this issue Aug 26, 2023 · 8 comments
Labels
bug Something isn't working

Comments

@johnhungerford
Copy link
Contributor

Describe the bug
When the default zio-http layer is included in an application, it takes a surprising amount of time for the application to terminate after completing (~2 seconds).

To Reproduce
Steps to reproduce the behaviour:
build.sbt:

ThisBuild / scalaVersion     := "3.3.0"
ThisBuild / version          := "0.1.0-SNAPSHOT"
lazy val root = (project in file("."))
  .settings(
    name := "zio-issues",
    libraryDependencies ++= Seq(
      "dev.zio" %% "zio" % "2.0.16",
      "dev.zio" %% "zio-test" % "2.0.16" % Test,
      "dev.zio" %% "zio-http" % "3.0.0-RC2",
    ),
  )

Main.scala:

import zio.*
import zio.http.*

object Main extends ZIOAppDefault:
  override def run: ZIO[Environment & ZIOAppArgs & Scope, Any, Any] =
    (ZIO.service[Client] *> Console.printLine("Hello world")).provide(Client.default)
  1. Create a project with the above files
  2. Run sbt run

Expected behaviour
It should not take a noticeably longer time than if the program was just Console.printLine("Hello world") (which sbt reports as 1 s on my machine).

Actual behavior
It takes 3 s on my machine.

Desktop (please complete the following information):

  • OS: MacOs Monterey 12.6.8

Additional context
This is an issue when using zio-http with a CLI app (built as a graal native image). I'd like to use it as a client but the program hangs after running. sttp with a zio backend doesn't have the same issue so I've been using that, but I'd like to migrate to straight zio.

@johnhungerford johnhungerford added the bug Something isn't working label Aug 26, 2023
@dejvid
Copy link

dejvid commented Aug 26, 2023

Yes I can confirm that. Its really too slow to be usable.

@johnhungerford
Copy link
Contributor Author

Looks like it boils down to the event loop group layer:

object Main extends ZIOAppDefault:
  override def run: ZIO[Environment & ZIOAppArgs & Scope, Any, Any] =
    (ZIO.service[EventLoopGroup] *> Console.printLine("Hello world!"))
      .provideSome[Scope](ZLayer(zio.http.netty.EventLoopGroups.default))

it can be simplified further to:

object Main extends ZIOAppDefault:
  override def run: ZIO[Environment & ZIOAppArgs & Scope, Any, Any] =
    EventLoopGroups.make(ZIO.succeed(DefaultEventLoopGroup())) *> Console.printLine("Hello world!")

If EventLoopGroups.make(...) is wrapped in ZIO.scoped, the pause happens prior to "Hello world"

@vigoo
Copy link
Contributor

vigoo commented Aug 28, 2023

This means the graceful shutdown of the netty EventLoopGroup is responsible for this: https://netty.io/4.1/api/io/netty/util/concurrent/EventExecutorGroup.html#shutdownGracefully--

@johnhungerford
Copy link
Contributor Author

Right. Looks like zio-http is using shutDownGracefully() without parameters instead of

Future<?> shutdownGracefully(long quietPeriod,
                             long timeout,
                             TimeUnit unit)

shutDownGracefully() provides "sensible default values," meaning the quietPeriod is something like 2 seconds, meaning it only will shutdown after going 2 seconds without a new event.

I don't have a good sense of the importance of this measure more generally. My guess is this default makes sense for servers but the default client should have a short quietPeriod (100 ms?), and it should be configurable in Client.Config. I'd be happy to take a stab at it if there are no objections.

@vigoo
Copy link
Contributor

vigoo commented Aug 28, 2023

Sounds good to me!

@johnhungerford
Copy link
Contributor Author

Opened #2410

@johnhungerford
Copy link
Contributor Author

johnhungerford commented Aug 30, 2023

Fixed by #2410

However, if you need this feature right now without waiting for other libraries to adapt to the big changes to Http (e.g. tapir), I also found the following workaround:

import java.util.concurrent.TimeUnit

import zio.*
import zio.http.*
import zio.http.netty.NettyFutureExecutor
import zio.http.netty.client.NettyClientDriver

import io.netty.channel.{DefaultEventLoopGroup, EventLoopGroup}

object CustomClient:
   private val group: ZIO[Scope, Nothing, EventLoopGroup] =
      ZIO.acquireRelease(ZIO.succeed(DefaultEventLoopGroup()))(ev =>
         NettyFutureExecutor
            .executed(ev.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS))
            .orDie,
      )

   val default: ZLayer[Any, Throwable, Client] = ZLayer.scoped {
      group.flatMap { elg =>
         ZIO.service[zio.http.Client]
            .provideLayer(
               zio.http.Client.default.map { env =>
                  env.getDynamic[ClientDriver] match
                     case Some(NettyClientDriver(channelFactory, _, nettyRuntime, clientConfig)) =>
                        env.add[ClientDriver](NettyClientDriver(channelFactory, elg, nettyRuntime, clientConfig))
                     case _                                                                      => env
               },
            )
      }
   }

Note that this hard-wires the event-loop group to a default version. If you have a custom netty config, you'll have to update this to reflect that configuration. (You could just copy over zio.netty.EventLoopGroups and adapt its make method to include ev.shutdownGracefully(0, 100, TimeUnit.MILLISECONDS)).

@ghost
Copy link

ghost commented Oct 2, 2023

@johnhungerford , issue looks closable (260cc73 merged)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants