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

Update docs for BlazeClientBuilder #3410

Merged
merged 2 commits into from Jun 23, 2020

Conversation

jgogstad
Copy link
Contributor

@jgogstad jgogstad commented May 11, 2020

Make it explicit that ExecutionContext.global should be used

Based on the discussion in #3330, it sounds to me that http4s requires this parameter because of a leaky implementation detail in Blaze, and that the right answer always is ExecutionContext.global. It doesn't hurt to point this out in the scaladoc.

A side note here: I see that the execution context is used to submit tasks to an ExecutorService. If ExecutionContext.global only has one thread, is it still safe to use it without risking deadlocks? I assume this is one of the reasons cats-effect/IOApp defaults to minimum two threads backing the ContextShift.

Make it explicit that ExecutionContext.global should be used
Copy link
Member

@rossabaker rossabaker left a comment

Oh, wow. I thought global defaulted to at least two, but it looks like not. Yes, that would be a good additional warning to give.

@rossabaker rossabaker added the docs Relates to our website or tutorials label May 11, 2020
@jgogstad
Copy link
Contributor Author

@jgogstad jgogstad commented May 11, 2020

At this point BlazeClientBuilder could just take Blocker as a parameter instead of ExecutionContext though. As long as we're not fixing the underlying problem with Blaze requiring an ExecutionContext. It's safe to assume that most clients will have a ContextShift and a Blocker. It would be less ergonomic to advice them to construct a (most likely) blaze-specific lower-bounded-2 thread pool just to cater to this implementation detail. Clients would most likely pass Blocker down to the call site anyway and pass the underlying thread pool to BlazeClientBuilder.

Also, if we advice clients not to use global, we should update the http4s docs to not use it too.

What do you think?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 11, 2020

Blocker[F] gives a cached (unbounded) thread pool, which is not intended for the CPU-intensive work that happens here. I think a great deal of apps don't have a blocker at all. I wish we could just use the ContextShift, but there is no way to turn that back into an ExecutionContext.

So the two options:

  • global
    • ergonomic
    • plays nicely with other Future libraries
    • dangerous on a single-core machine
    • could generate warning on those, but it still doesn't prevent runtime calamity
  • fixed thread pool with lower bound

In practice, I've always used global and never had a problem, but I think it's good that we're thinking about this more critically.

@rossabaker rossabaker added the RFC Design ideas that we'd like to spur discussion label May 11, 2020
Copy link
Member

@rossabaker rossabaker left a comment

I think it's long past time to give people an easy and correct enough answer. If we want a fixed thread pool, we need to point them to an easier way to create one in subsequent work.

@jgogstad
Copy link
Contributor Author

@jgogstad jgogstad commented Jun 22, 2020

I think recommending ExecutionContext.global is fine, it will work in most cases. We've had some deadlocks using global in high scale execution environments, like Spark or Beam jobs, where the number of VMs is large, but the size of each VM is on the cheaper end of the cloud provider's machine type spectrum. I've also encountered it when testing various cloud services, then also on cheap machine types.

It sounds to me like an upstream change in cats-effect is the only way to make this ergonomic and safe.

@rossabaker rossabaker merged commit 154468c into http4s:master Jun 23, 2020
10 checks passed
@jgogstad jgogstad deleted the blaze-client-builder-docs branch Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Relates to our website or tutorials RFC Design ideas that we'd like to spur discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants