Clear up blocking _in_ a future vs blocking _on_ a future. #303

backuitist · 2014-02-19T09:37:12Z

No description provided.

SethTisue · 2015-07-13T18:52:08Z

this is an edit to SIP-14. could one or more of the SIP's authors review it? @phaller @axel22 @heathermiller @viktorklang @viktorklang @rkuhn @vjovanov

axel22 · 2015-07-13T19:00:53Z

overviews/core/_posts/2012-09-20-futures.md

 ## Blocking

+### in a Future


Please put a capital I in In here.

I would maybe write Blocking inside a future.

axel22 · 2015-07-13T19:07:48Z

Thanks a lot for your contribution and effort to clear this up!

LGTM overall, please address the few details I commented.

Btw, there is another futures doc where this should be added:

http://docs.scala-lang.org/overviews/core/futures.html

SethTisue · 2015-07-14T03:12:51Z

@axel22 sorry, I was confused and managed to confuse you as well. this text derives from SIP-14, but it is not the SIP itself. the PR does in fact target the right page.

axel22 · 2015-07-14T07:24:26Z

@SethTisue even better, then, less work for the @backuitist !

viktorklang · 2015-07-14T15:47:14Z

overviews/core/_posts/2012-09-20-futures.md

+
+As seen with the global `ExecutionContext`, it is possible to notify an `ExecutionContext` of a blocking call with the `blocking` construct.
+The implementation is however at the complete discretion of the `ExecutionContext`. While some `ExecutionContext` such as `ExecutionContext.global`
+implement `blocking` by means of a `ManagedBlocker`, some just do nothing:


Perhaps be clear why this is the case:

"some will do nothing, like Executors.newFixedThreadPool, as shown below:"

This paragraph is about ExecutionContext and Executors.newFixedThreadPool isn't one. You're talking about ExecutionContext.fromExecutor(Executors.newFixedThreadPool(x)) which in this sentence is overkill IMO.
What I could add though, is a comment in the code below:

// an ExecutionContext not aware of `blocking`

You could put that long expression into a separate line - like this:

Some execution contexts, such as the fixed thread pool: ... long expression .... will do nothing, as shown in the following:

It's a bit redundant IMO but if you think that helps I'll add it.

SethTisue · 2015-07-20T16:11:00Z

@viktorklang ready to merge?

viktorklang · 2015-07-20T18:34:39Z

overviews/core/_posts/2012-09-20-futures.md

+    implicit val ec = ExecutionContext.fromExecutor(
+                        Executors.newFixedThreadPool(4))
+    Future {
+      // here blocking serves only for documentation purpose


Rereading the PR,
I think it is important to let the reader know that generally speaking: code using an ExecutionContext should not know implementation details regarding the ExecutionContext, and therefore any code that invokes blocking does so to let the ExecutionContext deal with the situation in the best way it knows how to. I.e. it does not serve as documentation purpose. (If the readers think that it is somehow optional and only serves as documentation, then it's not really going to be used.)

Does that line of thinking make sense?

Hi Viktor,
I was actually quoting you when I wrote that blocking serves for documentation purpose: https://groups.google.com/d/msg/scala-user/LKiVD2PGvew/CWufs2Z8K4UJ

If I use ExecutionContext.fromExecutor(Executor) the blocking construct will be made useless.

It's not useless. not only can you install your own BlockContext but it serves as "documentation" so that if someone changes the EC implementation, your code can be made to run better.

Hi Bruno,

Yes, in the case that the implementation behind the EC (or the currently installed BlockContext to be more precise) does not do anything, it is "effectively" documentation, but given that the EC typically is passed in, it may not be like that for all invocations.

My reservation is against making it sound like it is something that only serves documentation purposes, even if the EC implementation is known, because if the implementation changes in the future, it will be hard to track the blocking if that wasn't added when the code was written.

Does that make sense? :)

I haven't really changed my point of view on this matter since last year. IMO if you want to block you don't do it through a blocking call but rather you'd use an EC that has been designed for that, such as a TPE with a large pool.

if the implementation changes in the future

Do you really believe that we'll see in the (foreseeable) future an EC implementation for TPE that can leverage blocking?

In the mean time, this feature is dangerous and the least you can do is warn your users. I believe this PR addresses that perfectly.

Maybe @axel22 or @SethTisue could have a word on it?

I don't have a very strong opinion. Maybe we can omit the line which says that it's for documentation, and just say that it informs the EC so that it can optionally adjust its parallelism.

@backuitist

You're right, the library should not instantiate its own EC, but that's not what I said. Libraries needing a blocking EC can merely declare it as a dependency, and if you want to leverage implicits you can do something along those lines:
[...]
As a matter of fact I believe that, as a general rule, implicit parameters are fine as long as they have a very specific meaning. blocking feels like a hack to work around the fact that we only have one kind of ExecutionContext, not 2.
But that's just my opinion, and for now I'm just suggesting that we make it clear that blocking might not work as expected under certain conditions.

I can understand that argument, I think we discussed having multiple ones initially but came to the conclusion that it was disruptive enough to have the ripple-effect of having to add (implicit ec: ExecutionContext) up the call chain as soon as something async needed to be done down the call chain. Adding a new dimension where one also needed to add yet another implicit as soon as blocking was involved would have serious impact on binary and source compatibility. (Imagine having to add a blocking call, now all your signatures would change, so people would just do unmanaged blocking instead, which would mean that they'd block on the provided EC, without any means of parrying for it).

My long-standing plan has been to create a thread pool that would work well for both CPU-bound work á la FJP but also blocking work á la TPE, transitioning workers between those modes based on blocking/BlockContext. What would be fantastic about that is that code that uses blocking would simply work with the new EC without any modification.

I took a quick look. That's nice, but that's not yet in the scala-library and, as far as I understand, that only works if you create your TPE-based EC through your ThreadPoolConfig. If someone has a TPE at hand (provided by some other code) and wants to wrap it in an EC through fromExecutor then you're stuck.

Of course, it is not a virus that infects other's object instances :)

My long-standing plan has been to create a thread pool that would work well for both CPU-bound work á la FJP but also blocking work á la TPE, transitioning workers between those modes based on blocking/BlockContext. What would be fantastic about that is that code that uses blocking would simply work with the new EC without any modification.

That's fair, but how about leaving this PR as it is now and updating it once the scala-library gets upgraded with your silver bullet EC?

@backuitist Nice try ;)

Since there is nothing that prevents, nor precludes that such a threadpool already exists and is used in production daily, I think this PR should be amended such that it is clear that the primary thing about blocking is about demarcation rather than mere documentation :)

Everything else looks fantastic, it's a great contribution!

@viktorklang I surrender! ;)

How should we go about it? Do like @axel22 proposal? Would that work for you to remove that line?

Deal, I think @axel22's proposal is a good middle ground.

viktorklang · 2015-07-20T18:35:07Z

@SethTisue Thanks for the ping. I have one final comment (added).

Clear up blocking _in_ a future vs blocking _on_ a future.

SethTisue · 2015-07-23T14:24:16Z

thank you @backuitist for the submission and for getting it past our tough pack of reviewers :-)

Clear up blocking _in_ a future vs blocking _on_ a future.

307ab00

SethTisue changed the title ~~Clear up blocking _in_ a future vs blocking _on_ a future.~~ Jul 13, 2015

axel22 reviewed Jul 13, 2015
View reviewed changes

SethTisue changed the title ~~SIP-14: Clear up blocking _in_ a future vs blocking _on_ a future.~~ Jul 14, 2015

viktorklang reviewed Jul 14, 2015
View reviewed changes

backuitist force-pushed the blocking-in-future branch from 898a8bf to 4f768e0 Compare July 14, 2015 17:03

viktorklang reviewed Jul 20, 2015
View reviewed changes

addressing review comments

e61c7f5

backuitist force-pushed the blocking-in-future branch from 4f768e0 to e61c7f5 Compare July 21, 2015 16:21

SethTisue added a commit that referenced this pull request Jul 23, 2015

Merge pull request #303 from backuitist/blocking-in-future

eb07b9f

Clear up blocking _in_ a future vs blocking _on_ a future.

SethTisue merged commit eb07b9f into scala:master Jul 23, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Clear up blocking _in_ a future vs blocking _on_ a future. #303

Clear up blocking _in_ a future vs blocking _on_ a future. #303

Uh oh!

backuitist commented Feb 19, 2014

SethTisue commented Jul 13, 2015

axel22 Jul 13, 2015

axel22 Jul 13, 2015

axel22 commented Jul 13, 2015

SethTisue commented Jul 14, 2015

axel22 commented Jul 14, 2015

viktorklang Jul 14, 2015

backuitist Jul 14, 2015

axel22 Jul 14, 2015

backuitist Jul 14, 2015

SethTisue commented Jul 20, 2015

viktorklang Jul 20, 2015

backuitist Jul 20, 2015

viktorklang Jul 20, 2015

backuitist Jul 21, 2015

axel22 Jul 21, 2015

viktorklang Jul 21, 2015

backuitist Jul 21, 2015

viktorklang Jul 21, 2015

backuitist Jul 21, 2015

viktorklang Jul 21, 2015

viktorklang commented Jul 20, 2015

SethTisue commented Jul 23, 2015

Clear up blocking _in_ a future vs blocking _on_ a future. #303

Clear up blocking _in_ a future vs blocking _on_ a future. #303

Uh oh!

Conversation

backuitist commented Feb 19, 2014

SethTisue commented Jul 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axel22 commented Jul 13, 2015

SethTisue commented Jul 14, 2015

axel22 commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SethTisue commented Jul 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorklang commented Jul 20, 2015

SethTisue commented Jul 23, 2015