Added Blocker, a new type for blocking execution contexts

[mpilquist]

Fixes #555

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@482ae5e). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #556   +/-   ##
=========================================
  Coverage          ?   88.79%           
=========================================
  Files             ?       75           
  Lines             ?     2124           
  Branches          ?      131           
=========================================
  Hits              ?     1886           
  Misses            ?      238           
  Partials          ?        0

[SystemFw]

Looks great.
The only thing I would add is maybe a line to the scaladoc to somehow convey that you need to have application level bounds since this is backed by a cached threadpool. Even just a link to Daniel's gist would do.

[mpilquist]

Any opinions on an alternative constructor that uses a semaphore to guard calls to `eval`? Like `BoundedLinebacker`

…

On Sat, Jun 8, 2019 at 9:18 AM Fabio Labella ***@***.***> wrote: *@SystemFw* approved this pull request. Looks great. The only thing I would add is maybe a line to the scaladoc to somehow convey that you need to have application level bounds since this is backed by a cached threadpool. Even just a link to Daniel's gist would do. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#556?email_source=notifications&email_token=AAA42PWCBUPNMDQADKRLLNTPZOWRNA5CNFSM4HWHF7J2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB27IOUY#pullrequestreview-247367507>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAA42PRJHFRX46XEI6VUB4TPZOWRNANCNFSM4HWHF7JQ> .

[rossabaker]

With BoundedLinebacker, tasks just queue up, without bound or timeout? I can see how it elegantly protects the blocking resource, but it introduces failure scenarios of its own I think it's neat, but am mildly 👎 since there is a wide open design space for backpressure, and changing our APIs is expensive.

[rossabaker]

But 👍 on Blocker. There are a lot of ways to use it, but having it to use would be nice.

[rossabaker]

Note that this is a graceless shutdown. It may be useful to have a version that takes a timeout and awaits termination until the timeout. And since there are various things we might want to do with the returned list of runnables, I think unsafeFromExecutionContext carries its weight.

[mpilquist]

Yeah, that was intentional in this case. The pool should be empty at the point of finalization or it's arguably a bug in the caller's code. Also, we don't have a safe way to shutdown with a timeout unless we were to spawn a thread dedicated to shutdown.

[rossabaker]

I'm squeamish about the discard of the return value of shutdownNow. I'm thinking a non-empty list of unrrun runnables should maybe be raised into an error.

Shutdown with a timeout can be done without a new thread by the awaitTermination call, which would, uh, block off the Blocker. It's true that there's no async listener. I'm coming around to it as a bad idea.

[mpilquist]

Right, awaitTermination would block a thread in the main CPU bound thread pool, which is problematic. Raising an error if task list is non-empty is probably a good idea. Think that should be controllable via a flag or just always the behavior?

[rossabaker]

I think always. It's easily recoverable, but users can't do anything about it unless we surface it. Something that extends IllegalStateException with a NonEmptyList of Runnables feels right to me.

[mpilquist]

Here's what FS2 looks like if using Blocker: mpilquist/fs2@topic/unblock...topic/unblock-plus-blocker

[SystemFw]

Here's what FS2 looks like if using Blocker:

I like that. As a bonus, places where you genuinely need an Ec not for blocking (e.g. BlazeBuilder) now are clearly different

[mpilquist]

Any opinions on the names of unsafeFromExecutionContext and unsafeFromExecutorService versus wrapExecutionContext and wrapExecutorService or liftExecutionContext and liftExecutorService?

[djspiewak]

I think lift or liftExecutionContext are fine names. I don't think it merits the unsafe prefix, since the unsafety was technically what the user did to create the "bare" EC in the first place.

[djspiewak]

This, however, might merit the word "unsafe". Perhaps unsafeContext? The point being that you want to discourage people from unwrapping it directly unless they really know what they're doing.

[mpilquist]

How about blockingContext? I want to avoid making it difficult to integrate with APIs that take a plain ExecutionContext with an API note saying it should support blocking tasks (e.g. our very own ContextShift).

[djspiewak]

That's fair. Do you think we should also add an extra function to ContextShift which takes a Blocker and deprecate the ExecutionContext one? It feels weird to have two not-well-integrated parts of the API.

[mpilquist]

I thought so yeah, but then didn't do it to avoid breaking compatibility. But since this is a 2.0.0 release...

[mpilquist]

OK pushed a change -- I did NOT deprecate the EC variant of evalOn. See the ScalaDoc as to why.

[djspiewak]

I kinda prefer the name evalOn, just to avoid having eval and evalOn which both do the same thing.

[djspiewak]

We honestly might want to make the threads daemon, because there's a rather ugly situation that can arise here where someone has a stuck thread, the Resource scope closes, this error is thrown, the main thread halts, and then the program… hangs.

[djspiewak]

wrap is also fine. :-)

[mpilquist]

All review comments are addressed -- let me know if I missed anything.

[mpilquist]

JavaConverters is deprecated in 2.13 and I don't want to complicate the build with 2.12/2.13 source folders over 1 conversion from a Java to Scala list.

[jdegoes]

@mpilquist

👍 for attempting to deal with a common scenario for async effect systems

👎 for contaminating the Scala.js API with something that only makes sense for and runs on the JVM

👎 for overstepping into effect-specific territory (neither Monix nor ZIO need this, so it's really a Cats IO-specific feature) and, within the Cats ecosystem, encroaching into linebacker territory

👎 for being too volatile an area to rapidly codify into 2.0 (the issue was created 3 days ago), especially without substantial input or discussion from contributors to 3rd party effect types

Overall, 👎 . It would be better IMO to rally around linebacker for the duration of Cats 2.0, for example, which is free to specialize on the JVM and evolve independently from Cats Effect; or to rethink this in a way that makes sense for Scala.js and has buy-in from Monix, ZIO, and other effect communities.

[mpilquist]

@jdegoes It contaminates the Scala.js API and steps in to effect specific territory no more so than ContextShift#evalOn. We're stuck with evalOn for now and this only improves usability of the existing design.

[ChristopherDavenport]

The point of cats(-effect) is a set of abstractions that others can share and buy-in to if they would like to.

3rd party effect types are free to engage in discussion, but the progress of the library is independent of any implementations.

[rossabaker]

One bikeshed of the exception below, but looks good.

[rossabaker]

[GitHub ate my review, retrying]

I like raising an error when Blocker closes with outstanding tasks, but should we let the global compute pool in IOApp exit successfully in the same scenario? A difference is that this is recoverable and that's not. I don't know what we would do with the NEL in IOApp other than exit non-zero and maybe log the (probably uselessly toStringed) runnables.

I bring it up here, wondering whether this should be nested or has other uses. But even if we change the behavior of IOApp, it's hard to see that the exception is useful there.

[mpilquist]

That's a fair point, although evalOn works with "identity semantics" (there is only one execution context on Javascript), whereas this one will not.

Sure it will, just pass Blocker.liftExecutionContext(global) on Scala.js.

It contaminates the Scala.js API with irrelevant and misleading clutter and will blowup at runtime.

You mean specifically the constructor that creates a thread pool? I'm OK with moving that to JVM only.

Why not just move it over into jvm? We can do that with extension method and preserve the same API.

FS2 has a couple of places in the core project that took a blockingExecutionContext to wrap things like printing to the console. Passing global there is harmless so I'd like to see this stay in both JVM and JS.

Again, this isn't a new type class -- there's already ContextShift instances for Monix (and presumably ZIO) so I don't think the last-minute instability comments are applicable.

[mpilquist]

I moved the JVM specific constructors to JVM source directory: 57edf53

[djspiewak]

That's a fair point, although evalOn works with "identity semantics" (there is only one execution context on Javascript), whereas this one will not.

That's up to whoever instantiates Blocker, as Mike points out.

If the purpose of Cats Effect has shifted to be a set of type classes around Cats IO, rather than an interop standard for different effect types—of which IO is only one example—then I'm sure a lot of people in the community would like to know that.

This is not a typeclass.

The discussion of the conflation of the two concerns (a concrete datatype and a set of more abstract typeclasses) is a real one with a lot of nuance and merit, and IMO should be continued on the cats-effect 3 issue. We are where we are today, however.

It contaminates the Scala.js API with irrelevant and misleading clutter and will blowup at runtime.

There is no runtime blow-up. I'll buy the clutter argument, but there's no need for hyperbole.

Why not just move it over into jvm? We can do that with extension method and preserve the same API.

Conditionally-compiled APIs is one of the worst things ever, particularly with top-level types. Conditionally-available functions are a little better, since for the most part you can just avoid calling them, but making Blocker the type a conditionally-extant entity will have a viral effect on every cross-built in the ecosystem.

I think that perhaps wrapping the ExecutorService should be conditionally-compiled, particularly as it is an introduction rule, but that's about the extent of what I think is reasonable.

More precisely, Cats Effect has a specific effect (Cats IO). This issue is evidence of further contamination of the type class hierarchy by the specific design characteristics of Cats IO.

Oh it would be… if it were in the typeclass hierarchy. Which it is not. Which you know. Please keep this discussion focused on the merits of this change, rather than on broader philosophical points. As I said, I'm interested in the discussion you're referencing, and I think it should be continued… on the cats-effect 3 discussion issue. Not here.

Indeed, the issue was created 3 days ago and is going to ship into a rushed 2.0 that will be with us for a very long period of time.

Cats Effect 2 is an interim step. Cats Effect 3 will break many of these things, likely including Blocker, and will be undertaken with commensurate care. "With us for a very long time" is perhaps true to a degree, due to the expected long-tail on series/2.x, but as this is part of the concrete API rather than the polymorphic, I don't feel the repercussions are particularly relevant to effect implementors, only middleware and end-users, both of whom have voiced support.

The insinuation of carelessness is also unwelcome, here and everywhere.

If I had heard significant objection to Blocker's inclusion in 2.0, I would have put my foot down. Instead I heard significant support, both here and elsewhere. If I had felt that this completely came out of no where without any conceptual proving in the wild, I would have not considered it. Instead, numerous people (including myself) came forward and noted that we had all rolled essentially the same utility internally in our various projects. This ticks off the last of the boxes, and satisfied me that this had matriculated sufficiently to merit inclusion in the concrete API (I would not have supported such a last-minute change to the abstractions, regardless of any proofing).

[jdegoes]

You mean specifically the constructor that creates a thread pool? I'm OK with moving that to JVM only.

Yes, exactly.

I moved the JVM specific constructors to JVM source directory:

I am happy to see that. 👍

FS2 has a couple of places in the core project that took a blockingExecutionContext to wrap things like printing to the console. Passing global there is harmless so I'd like to see this stay in both JVM and JS.

I can see the convenience, although shouldn't that be an implementation detail of a Console[F]? Anytime blocking/non-blocking semantics end up inside an API that can be used cross-platform, it's probably a sign there's a missing type class somewhere.

There is no runtime blow-up. I'll buy the clutter argument, but there's no need for hyperbole.

No hyperbole intended. By "blow up", I mean there were methods in the pull request that would throw an exception if invoked on Scala.js.

We have all the tools necessary to restrict JVM-only features to the JVM, and IMO we should use them (and I am happy to see 5c073ef).

Oh it would be… if it were in the typeclass hierarchy. Which it is not. Which you know.

Whether type class or not, this is a chunk of functionality whose value is intrinsically tied to use of the concrete Cats IO data type, and I bring it up because adding even more such IO-specific functionality into Cats Effect should be explicitly recognized as a design-smell.

The insinuation of carelessness is also unwelcome, here and everywhere.

I never said it was careless—I said it was rushed, which is not the same as careless. In the past, changes this large have taken more time and have involved input from Alex, myself, or both, and have considered more aspects of the ecosystem (such as Scala.js compatibility, utility for Monix and ZIO, etc.).

I feel if I had not seen this today (more or less chance!), it would already be merged in, without separation of JVM-only functionality, and this would have had a lasting impact on the API.

[SystemFw]

Whether type class or not, this is a chunk of functionality whose value is intrinsically tied to use of the concrete Cats IO data type,

You seem to conflate "ZIO does not need it" with "it's needed only for cats IO", forgetting polymorphic code, which is a primary design concern for cats-effect

[jdegoes]

@SystemFw

IMO a true effect-polymorphic solution would introduce a blocking combinator as part of a type class, which is a design possibility unexplored here and which I would have loved to see considered as an alternative. It's also more JS friendly.

[SystemFw]

IMO a true effect-polymorphic solution would introduce a blocking combinator as part of a type class,

Sure, but if you consider a bit of context you see that is just an improvement over current evalOn , which makes it strictly better for 2.0.

I'm sure blocking can be considered for 3.0, even though when we did discuss it in the past the decision to have a single Ec for all blocking tasks in an app seemed debatable at least, and therefore one could argue that blocking is just a special case of Blocker

[jdegoes]

Sure, but if you consider a bit of context you see that is just an improvement over current evalOn , which makes it strictly better for 2.0.

Well, this is precisely the kind of conversation I would have liked to have, or at least see someone else have. 😄

The following design would be truly effect polymorphic (i.e. no assumptions about implementation) and of use to non-IO types, as well as fully implementable for Cats IO:

trait Blocking[F[_]] {
  def blocking[A](fa: F[A]): F[A]
}

It also has better ergonomics and is easier to constrain to the JVM.

a single Ec for all blocking tasks in an app seemed debatable at least

Since we are literally talking about a JVM-only thread pool that has unbounded capacity, I don't think that argument has merit in the current context.

[mpilquist]

Whether type class or not, this is a chunk of functionality whose value is intrinsically tied to use of the concrete Cats IO data type, and I bring it up because adding even more such IO-specific functionality into Cats Effect should be explicitly recognized as a design-smell.

IMO a true effect-polymorphic solution would introduce a blocking combinator as part of a type class, which is a design possibility unexplored here and which I would have loved to see considered as an alternative. It's also more JS friendly.

Blocker improves the usability of the existing abstractions. Replacing ContextShift with a blocking combinator / first class support in a type class is on the table for CE 3 but not CE 2.

trait Blocking[F[_]] {
def blocking[A](fa: F[A]): F[A]
}

This is a new type class, which would require all effect libraries to opt-in. This is beyond the scope of 2.0 IMO.

Since we are literally talking about a JVM-only thread pool that has unbounded capacity, I don't think that argument has merit in the current context.

Note you're focused on the apply constructor but forgetting that evalOn is used for other things as well (e.g. working with a library that requires all calls to occur from a special library managed thread pool).

I never said it was careless—I said it was rushed, which is not the same as careless. In the past, changes this large have taken more time and have involved input from Alex, myself, or both, and have considered more aspects of the ecosystem (such as Scala.js compatibility, utility for Monix and ZIO, etc.).

Deadlines tend to produce PRs and this is by no means even close to the largest PR to hit just prior to a deadline. I'd describe this as a small PR, adding a newtype around ExecutionContext along with some convenience constructors & combinators.

More importantly, cats-effect issues tend to be bike shed more than any project I've seen. I worry that we are driving away contributors and current maintainers with these conversations.

[jdegoes]

Blocker improves the usability of the existing abstractions. Replacing ContextShift with a blocking combinator / first class support in a type class is on the table for CE 3 but not CE 2.

I don't want to replace ContextShift in 2.0, just I'd prefer to avoid adding machinery to it, and I'd be happy to support the Blocking type class in both Monix and ZIO, leaving only Cats IO, which given the infrastructure in this PR, would be pretty easy to support as well.

That said, fair enough!

Note you're focused on the apply constructor but forgetting that evalOn is used for other things as well (e.g. working with a library that requires all calls to occur from a special library managed thread pool).

This is exactly the kind of discussion I want to have. 😄

I'd point out that blocking does not replace the need for evalOn, it only replaces the #1 use case for this functionality, in a way that doesn't assume any implementation details.

Deadlines tend to produce PRs and this is by no means even close to the largest PR to hit just prior to a deadline.

I agree, and I saw similar urgency prior to CE 1.0 and appreciated the pushbacks.

More importantly, cats-effect issues tend to be bike shed more than any project I've seen. I worry that we are driving away contributors and current maintainers with these conversations.

Although you would expect and hope that in a widely depended upon library, changes would involve discussion and take some time to work through.

But you may be right about the bike-shedding, and I would propose the following:

Stop bike-shedding at the request of any Cats Effect contributor when any pull request / issue has:

• Buy-in from at least 2 distinct end-users of distinct libraries that depend on cats-effect
• Buy-in from distinct contributors to at least 2 different libraries that depend on cats-effect
• Buy-in from distinct contributors to at least 2 different effect data types that support cats-effect

The stake holders are the end-users, the library maintainers, and the effect maintainers, and if you have a little consensus in each category, it's probably a safe enough change to push through without additional bike-shedding.

[djspiewak]

We have all the tools necessary to restrict JVM-only features to the JVM, and IMO we should use them (and I am happy to see 5c073ef).

While I think that is true to a degree, it can also cause some significant heartache in a lot of cases when not done carefully, particularly when employed on top-level types. In this case, simply moving the constructor is certainly the right approach.

Whether type class or not, this is a chunk of functionality whose value is intrinsically tied to use of the concrete Cats IO data type, and I bring it up because adding even more such IO-specific functionality into Cats Effect should be explicitly recognized as a design-smell.

You continue to see Cats Effect as "abstraction primary, concrete secondary", and thus you view any API changes to Cats Effect which are not literally part of IO to be something that is targeted at the broader abstraction surface area. Again, this is a conversation for 3.0 planning, but I want to be very specific about calling out that mindset. You have a misconception about what Cats Effect is, based solely on your perspective as an external implementor and user of the API, and severely distorted by your needs from that API and your design goals, subjective and objective. This is all to be expected, but please be conscious of it as we move forward with 3.0 design discussions, or it will keep arising as a point of contention over and over and over.

To be clear, I'm not begrudging you your biases. I certainly have my own. What I am begrudging is your assumption that everyone shares yours.

In the past, changes this large have taken more time and have involved input from Alex, myself, or both, and have considered more aspects of the ecosystem (such as Scala.js compatibility, utility for Monix and ZIO, etc.).

I certainly considered Scala.js while reviewing this, though perhaps not with the thoroughness that I should have. Monix and ZIO always have a seat at this table (as does everyone, really), but in this case the input is less relevant. Alex is also extremely busy right now.

I feel if I had not seen this today (more or less chance!), it would already be merged in, without separation of JVM-only functionality, and this would have had a lasting impact on the API.

Probably not merged today, but if you hadn't seen it at all and no one else had raised the point about the thread pool constructor, then yes it likely would have been merged in that form. And that is precisely why I engage in these sorts of conversations: deep down, at the end of the day, some value will come out of it.

IMO a true effect-polymorphic solution would introduce a blocking combinator as part of a type class, which is a design possibility unexplored here and which I would have loved to see considered as an alternative. It's also more JS friendly.

Drifting into cats-effect 3 land, but unless datatypes have an intended implementation of blocking which does not directly boil down to evalOn(blockingEC) (assuming compositional evalOn semantics and the JVM), then I'm strongly in favor of saying that blocking just isn't a primitive. But it's at least an interesting design question.

Since we are literally talking about a JVM-only thread pool that has unbounded capacity, I don't think that argument has merit in the current context.

I can think of at least three cases within as many years where I have needed non-singleton pools for blocking tasks. At least one of them you were personally witness to. It's very common, particularly when interacting with older APIs written for the Java Enterprise Application Server era.

This is a new type class, which would require all effect libraries to opt-in. This is beyond the scope of 2.0 IMO.

It is.

More importantly, cats-effect issues tend to be bike shed more than any project I've seen. I worry that we are driving away contributors and current maintainers with these conversations.

This. Very much this. While I don't mind a lot of reasoned debate and careful consideration around even minor things like naming on a project which is as central as cats-effect, there is certainly a line that we need to be careful of.

Stop bike-shedding at the request of any Cats Effect contributor when any pull request / issue has: …

I more or less follow this process already when policing this community (granted, from which I took a long hiatus). The exception that I draw is that, on certain matters I lend considerably more weight to implementors of datatypes, on certain others I lend considerably more weight to middleware consumers (polymorphic libraries), and on a third set I lend more weight to end-user considerations (downstream consumers of polymorphic libraries and datatypes). Cats Effect's role is to mediate between all three of these, and some issues lend considerably more credence to one stakeholder over another.

But in general, yes, I think that if something has overwhelming and universal buy-in of the type you describe, then it is beyond a shadow of a doubt.

[jdegoes]

You have a misconception about what Cats Effect is, based solely on your perspective as an external implementor and user of the API

My biases, whatever they may be, don't invalidate my technical arguments that Blocker is overly specialized to Cats IO, rather than being truly effect polymorphic and effect type agnostic, like the proposed Blocking type class (which makes no assumptions regarding how the functionality is provided).

In fact, if at any point we find ourselves discussing other people's "biases" and "misconceptions" (which are personal judgments), rather than discussing technical issues, then I'd say we've veered off the happy path and should discontinue the conversation.

[kubukoz]

Why not a value class?

[mpilquist]

No reason really - I tend to avoid value classes unless the performance gain is significant. In this case, it's a single extra allocation per app in most cases. :)

[kubukoz]

Good point, OTOH it'd make sense if old-style evalOn was removed. But since it's not being removed... :)

[mpilquist]

See #560 btw

[kubukoz]

overloading is still something I'd try to avoid as long as possible... If this was blockOn, it'd probably make Blocker eligible for a value class, no? Plus more explicit than just an overload

[kubukoz]

Has anyone mentioned the possibility of taking the required type classes (Sync, ContextShift [yes I know it's not a type class]) at construction time, instead of at call sites of methods? That'd make Blocker[F] a thing and possibly reduce the amount of Sync necessary to use it in many places. And it wouldn't push any responsibility on the implementations like Monix and ZIO either.

[mpilquist]

@kubukoz That was intentional, as it causes problems when using multiple effects -- e.g. transformers plus a base effect type. The resource itself (the pool) doesn't care which F is used to submit tasks to it, so the current encoding is more flexible.

[kubukoz]

@mpilquist I see, so how about dual effects like in creation of a Ref and friends?

[mpilquist]

Same issue still -- e.g. imagine a Skunk app that uses a Blocker to talk to Postgres via TCP but also uses the same Blocker when writing files to disk in IO effect. Or similarly, an http4s app that does blocking IO in handling responses but also file IO outside request handling.

[kubukoz]

mapK then? You could have a single underlying EC and create a copy with a different effect just by mapping between them with a FunctionK (from IO to Kleisli or from IO to whatever Skunk needs)

[mpilquist]

Then you run in to an issue with mapK[G[_]](u: F ~> G) needing an implicit Sync[G] and ContextShift[G], which means it's not really mapK (this same issue exists in fs2 -- see translate vs translateInterrupt).

[mpilquist]

BTW, you also run in to the issue of changing which implicits have to be passed to various use sites -- something Blocking also shares. That is, with Blocker not depending on an F, you still need the same Sync[F] and ContextShift[F] at the call site. If Blocker was parameterized by F, many uses would change from needing those two type classes + an ExecutionContext to needing just a Blocker[F]. I think that's a big enough change to discount it for 2.0.