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

Added executionContext as implicit parameter to BlazeBuilder.apply #3330

Closed

Conversation

albertoadami
Copy link
Contributor

@albertoadami albertoadami commented Apr 15, 2020

Right now the BlazeBuilder.apply is using global as ExecutionContext by default.
In my opinion, is better to pass executionContext as an implicit parameter.
Please note that the official documentation is not totally clear about that, and from my point of view running an http4s server with global as the execution context in production is not correct.

@albertoadami albertoadami changed the title Added executionContext as implicit parameter to BlazeBuilder.apply, i… Added executionContext as implicit parameter to BlazeBuilder.apply Apr 15, 2020
@hamnis
Copy link
Contributor

@hamnis hamnis commented Apr 15, 2020

Being explicit with ExecutionContext's is a Good Thing (tm) IMHO

@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 15, 2020

@hamnis from my point of view is more correct to use something like the BlazeClientBuilder.apply.

Do you not agree?
Or at least the ExecutionContext should be more documented on the official documentation, from my point of view isn't totally clear that the default execution context is the global right now.

Like a http4s user, I'd prefer to construct the server with

BlazeServerBuilder[IO](executionContext)
than with:

        BlazeServerBuilder[IO]
          .withExecutionContext(executionContext)

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 18, 2020

I think Scala style has generally evolved toward using implicits for instances and syntax, and away from using it for dependencies. Certainly so in our FP corner of the universe. But ExecutionContext has a long tradition of being passed implicitly. So I think the tension here is about sticking to idiomatic usage of this specific type vs. correcting a historical mistake.

I wouldn't ever want to explicity call it BlazeServerBuilder[IO](executionContext), where it will be tied in with instance parameters. I think it's more of a question as to whether the default should be global vs. whatever implicit is in scope at the point of construction.

@SystemFw
Copy link
Member

@SystemFw SystemFw commented Apr 18, 2020

In my opinion, is better to pass executionContext as an implicit parameter.

I think this should be put in context. In a pure FP application, there will generally be no implicit ECs flying around, since everything is written in IO which manages pools differently than Future (for which expecting an implicit Ec would feel way more idiomatic than the default).
So, often,

whatever implicit is in scope at the point of construction.

amounts to none. In an ideal world we should be able to take an Ec out of ContextShift so that users don't have to worry about this implementation detail of Blaze, but since that is not possible, what do we gain by not defaulting to global?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 18, 2020

That's a good point. We'd break a lot of IOApps unless we made it an implicit with a default argument, which neither FP idiom nor standard library idiom endorse. The main beneficiaries of this change would be apps that create a non-default, implicit ExecutionContext and have incorrectly assumed it's implicitly threaded to blaze.

@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 18, 2020

From my point of view passing ExecutionContext as an implicit parameter is more complaint with the actual behavior of the BlazeClientBuilder.apply.
An important thing is to update the documentation with more details about the ExecutionContext because it's not totally clear if the user doesn't take a look at the library code.

Copy link
Member

@rossabaker rossabaker left a comment

I've changed my mind and believe @albertoadami is right in moving it to apply. @SystemFw is correct that many FP applications won't have an ExecutionContext except for this leaky implementation detail of blaze. However, by strictly referencing ExecutionContext.global in the constructor of the builder, we're creating global fork-join thread pool in pure code.

However, I think it should be an explicit argument to apply:

  1. Avoids the perils of implicit configuration.
  2. Would allow us to deprecate the existing signature and explain what's going on to everyone who upgrades. Callers can call BlazeServerBuilder[IO](ExecutionContext.global), or even BlazeServerBuilder[IO](implicitly).
  3. Most convincingly, matches what we already did in BlazeClientBuilder!

I think that the untracked thread pool creation is arguably enough of a bug, that if we go the deprecation route, we should even consider bringing this back to series/0.20.

@rossabaker rossabaker added the bug label Apr 20, 2020
@rossabaker rossabaker added this to the 0.20.22 milestone Apr 20, 2020
@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 21, 2020

@rossabaker if I understand correctly your comment, the changes that I need to do are the following:

  1. mark the actual apply implementation as @deprecated
  2. add a BlazeServerBuilder.apply(executionContext) method with the explicit executionContext and an other one with the implicit argument(basically what I've done in this PR)

If it's totally correct what I'm saying I could work on this changes in the next couple of days.

@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 22, 2020

I have some integrations about what I wrote above:

  1. also BlazeServerBuilder.withExecutionContext should become a @deprecated method?
  2. I think that the BlazeServerBuilder.apply cannot be overloaded using different kind of implicit parameters, so we need to decide if use an explicit argument or implicit.

@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 22, 2020

@rossabaker I refactor some code, I see that there are some error on CI on specific java versions, can you help me on this?

rossabaker
rossabaker previously approved these changes Apr 26, 2020
Copy link
Member

@rossabaker rossabaker left a comment

I think this is good. Marking as retarget, so we remember to cherry-pick it back to series/0.20 instead of merge to master.

I don't know that we need a version that takes it implicitly. BlazeClientBuilder doesn't, and calling it as implicitly will summon an implicit, for whoever is managing their execution contexts that way.

withExecutionContext gets a little less interesting, since it would only be used to change whatever was provided by the constructor. But these builder patterns typically provide "setters" for all the values. It's conceivable to have a builder that represents a different set of defaults within an organization, and can still be customized in an app. So I don't see a need to deprecate that.

The Java 1.14 tests were fixed in #3350, I believe. And the other error is #3338.

@rossabaker rossabaker added the retarget label Apr 26, 2020
@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 26, 2020

@rossabaker I removed the @deprecated annotation from the withExecutionContext method.
I think that the implicit executionContext as argument is not necessary, the client is doing the same thing right now.

About the #3350 and #3338 errors, should I merge the master branch inside this one?

@rossabaker rossabaker dismissed their stale review Apr 26, 2020

tut failing

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 26, 2020

Oops, I missed that you'd already deprecated withExecutionContext when I approved last night.

I'm going to need to cherry-pick this onto a maintenance branch anyway. It will be a little easier if we don't have a merge in it. You could rebase if you like, or we can just ignore those failures we already know to be fixed. Whatever is easier for you.

I think this is ready as soon as we fix the docs. I wouldn't be surprised if there are a couple more, since tut fails fast:

[tut] *** Error reported at /home/runner/work/http4s/http4s/docs/src/main/tut/json.md:277
<console>:44: warning: method apply in object BlazeServerBuilder is deprecated (since 0.20.22): Use BlazeServerBuilder.apply with explicit executionContext instead
       val server = BlazeServerBuilder[IO].bindHttp(8080).withHttpApp(jsonApp).resource

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 26, 2020

Close!

[tut] compiling: /home/runner/work/http4s/http4s/docs/src/main/tut/service.md
[tut] *** Error reported at /home/runner/work/http4s/http4s/docs/src/main/tut/service.md:135
// <console>:37: warning: method apply in object BlazeServerBuilder is deprecated (since 0.20.22): Use BlazeServerBuilder.apply with explicit executionContext instead
//        val serverBuilder = BlazeServerBuilder[IO].bindHttp(8080, "localhost").withHttpApp(httpApp)

@albertoadami
Copy link
Contributor Author

@albertoadami albertoadami commented Apr 26, 2020

@rossabaker now should be all ok.

Copy link
Member

@rossabaker rossabaker left a comment

Thanks for sticking with it through the initial skepticism and the unrelated test instability. I think after all that, we got a positive change that increases consistency.

@@ -12,6 +12,7 @@ import org.http4s.{Http4sSpec, HttpApp}
import scala.concurrent.duration._
import scala.io.Source
import scala.util.Try
import scala.concurrent.ExecutionContext.Implicits.global
Copy link
Contributor

@satorg satorg Apr 27, 2020

Choose a reason for hiding this comment

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

If it's expected to use global explicitly only, it's usually better to

import scala.concurrent.ExecutionContext.global

is there a reason to import the implicit global?

Copy link
Contributor

@satorg satorg Apr 27, 2020

Choose a reason for hiding this comment

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

As I can see, some code below imports the implicit global, some other – the explicit one. But I cannot figure out the difference. Is seems to me that global is referenced explicitly everywhere, isn't it?

Copy link
Member

@rossabaker rossabaker Apr 27, 2020

Choose a reason for hiding this comment

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

That's true. They point to the same thing, and we probably imported from Implicits when this ticket originally set out to make it implicit. But it would be marginally better if it just came from ExecutionContext.global. I can mop that up when I backport it to the right branch, unless @albertoadami beats me to it.

Copy link
Contributor Author

@albertoadami albertoadami Apr 27, 2020

Choose a reason for hiding this comment

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

@rossabaker I can do the changes directly to this PR here, it's a 5 minutes work.
I think that's better to use the scala.concurrent.ExecutionContext.global.

Copy link
Contributor Author

@albertoadami albertoadami Apr 27, 2020

Choose a reason for hiding this comment

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

I changed the global import inside the tests, but now I get a test error on Scala 2.12.11, Java adopt@1.8 CI, but shouldn't be due to my commit.

rossabaker pushed a commit to rossabaker/http4s that referenced this issue Apr 27, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 27, 2020

When this gets its second signoff, eaaf50b will apply cleanly to the series/0.20 branch so we can include it in 0.20.22.

hamnis
hamnis approved these changes Apr 27, 2020
rossabaker added a commit that referenced this issue Apr 27, 2020
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Apr 27, 2020

This is now on the series/0.20 branch.

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

Successfully merging this pull request may close these issues.

None yet

5 participants