-
Notifications
You must be signed in to change notification settings - Fork 803
Document the use of the execution context passed to the blaze builders. #6098
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
Document the use of the execution context passed to the blaze builders. #6098
Conversation
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theHttpApp
". reqLoopCallback
gets called onblaze-selector-#
runRequest
gets called onio-compute-#
renderResponse
gets called onio-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.
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. |
There was a problem hiding this 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.
I'm going to cherry-pick this back to 0.23 before merge. It's all true there. |
52a55b3
to
8d3310a
Compare
…c-docs Document the use of the execution context passed to the blaze builders.
Following a discussion on discourse, I'd like to improve the docs on the custom execution context provided to the
BlazeServerBuilder
andBlazeClientBuilder
.Let me know what you think.
Thanks!