Better Timeouts, Valid Client Connections #4301
Conversation
| @@ -226,7 +216,8 @@ object EmberClientBuilder { | |||
| val acgFixedThreadPoolSize: Int = 100 | |||
| val chunkSize: Int = 32 * 1024 | |||
| val maxResponseHeaderSize: Int = 4096 | |||
| val timeout: Duration = 60.seconds | |||
| val idleReadTime = org.http4s.client.defaults.RequestTimeout | |||
There was a problem hiding this comment.
Bringing attention to what these values should be.
ember-client/src/main/scala/org/http4s/ember/client/internal/ClientHelpers.scala
Outdated
Show resolved
Hide resolved
ember-server/src/main/scala/org/http4s/ember/server/internal/ServerHelpers.scala
Outdated
Show resolved
Hide resolved
ember-client/src/main/scala/org/http4s/ember/client/EmberClientBuilder.scala
Outdated
Show resolved
Hide resolved
| case Right((request, response)) => send(socket)(Some(request), response) | ||
| case Left(err) => | ||
| Stream | ||
| .unfoldLoopEval(reachedEndError(socket))(s => |
ember-server/src/main/scala/org/http4s/ember/server/internal/ServerHelpers.scala
Outdated
Show resolved
Hide resolved
ember-server/src/main/scala/org/http4s/ember/server/internal/ServerHelpers.scala
Outdated
Show resolved
Hide resolved
| @@ -40,40 +40,12 @@ final class EmberClientBuilder[F[_]: Concurrent: Timer: ContextShift] private ( | |||
| private val logger: Logger[F], | |||
| val chunkSize: Int, | |||
| val maxResponseHeaderSize: Int, | |||
| private val idleConnectionTime: Duration, | |||
There was a problem hiding this comment.
We've never been consistent about what's a public val on a builder and what isn't.
There was a problem hiding this comment.
We have not. We'd need to be consistent in our configurations. I just worry that I still have my timeouts wrong and wanted to give myself wiggle room in case I screwed up.
ember-client/src/main/scala/org/http4s/ember/client/EmberClientBuilder.scala
Outdated
Show resolved
Hide resolved
| idleConnectionTime, | ||
| timeout, |
There was a problem hiding this comment.
I wouldn't mind some named arguments here. These are both durations or finite durations, right?
| @@ -156,6 +156,7 @@ final class EmberServerBuilder[F[_]: Concurrent: Timer: ContextShift] private ( | |||
| ) | |||
| _ <- Resource.make(Applicative[F].unit)(_ => shutdown.await) | |||
| _ <- Resource.liftF(ready.get.rethrow) | |||
| _ <- Resource.liftF(logger.info(s"Ember-Server service bound to address: $bindAddress")) | |||
There was a problem hiding this comment.
Blaze printed out a pretty crazy banner, do we want to do something like that in Ember also?
There was a problem hiding this comment.
I vote yes. I did it as a joke, and people absolutely loved it. Like, some of the most positive feedback we've ever gotten.
THE PEOPLE DEMAND THEIR FIGLETS.
There was a problem hiding this comment.
(Unlike the potential deadlock I fretted about above, I don't want to block this on a figlet.)
ember-client/src/main/scala/org/http4s/ember/client/EmberClientBuilder.scala
Outdated
Show resolved
Hide resolved
No description provided.