Skip to content

Conversation

zainab-ali
Copy link
Contributor

Following a discussion on discourse, I'd like to improve the docs on the custom execution context provided to the BlazeServerBuilder and BlazeClientBuilder.

Let me know what you think.

Thanks!

@@ -209,6 +209,18 @@ class BlazeServerBuilder[F[_]] private (
override def bindSocketAddress(socketAddress: InetSocketAddress): Self =
copy(socketAddress = socketAddress)


/** Configures the compute thread pool used to process requests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m not too happy with the wording here — “process requests” is pretty ambiguous. That said, I can’t think of a better term.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's good. I came up with "responding to requests", but is that calling the database, or is that just serializing a response body? My term is worse.

* for picking up tcp connections which is completely separate to
* this pool. Following picking up a tcp connection, Blaze shifts
* to this compute pool to process requests. Request processing
* logic is specified by the `HttpApp`.
Copy link
Contributor Author

@zainab-ali zainab-ali Mar 9, 2022

Choose a reason for hiding this comment

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

Request processing logic is specified by the HttpApp

On re-reading, I don't think this is correct. The handlers specified by HttpApp[IO] would run on the Async[F].executionContext pool instead, wouldn't they. It would just be Futures defined internally to blaze that would run on this custom execution context.

It might be simpler to say that this is a compute pool for running async computations and leave it at that, without going into detail on what exactly is run.

Copy link
Member

Choose a reason for hiding this comment

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

I understood this deeply a few effect systems ago, but had to do some experiments now! Using withExecutionContext(scala.concurrent.ExecutionContext.global), with this route:

      case GET -> Root / "thread" =>
        F.delay(Thread.currentThread.getName).flatMap(Ok(_))
  • The response body is io-compute-#. I'd say this is the "logic specified by the HttpApp".
  • reqLoopCallback gets called on blaze-selector-#
  • runRequest gets called on io-compute-#
  • renderResponse gets called on io-compute-#.
  • The effect of writing the body with bodyEncoder gets called on io-compute-#. req loop callback gets called again on the same thread.

So what actually does happen on the ExecutionContext?

  • The runnable after we have parsed a request, but we were already on a compute pool.
  • Some scheduled tasks, like the idle timeout
  • Probably some onComplete handlers in blaze that I can't trace from this repo
  • Every EntityBodyWriter has one, but I compiled without it!

It's a lot less useful than it used to be! I think the less-detailed description is better for now, and we might be able to rip out more places it's used with the advances in CE3.

@rossabaker
Copy link
Member

The deeper I probe into this on Cats-Effect 3, the harder I'm finding it to justify making it distinct from the default. This might be a candidate for deprecation and removal.

@zainab-ali zainab-ali marked this pull request as ready for review March 21, 2022 14:34
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I think this is an improvement, and it also inspired some cleanups in linked tickets. Thanks!

I'm still somewhat serious about removing the configuration, but that's probably a discussion for main.

@rossabaker
Copy link
Member

I'm going to cherry-pick this back to 0.23 before merge. It's all true there.

rossabaker added a commit to http4s/blaze that referenced this pull request May 26, 2022
…c-docs

Document the use of the execution context passed to the blaze builders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants