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

Cats Effect 2 executor doesn't shift back to the compute pool #2445

Open
pjurczenko opened this issue Jun 6, 2021 · 5 comments
Open

Cats Effect 2 executor doesn't shift back to the compute pool #2445

pjurczenko opened this issue Jun 6, 2021 · 5 comments
Labels

Comments

@pjurczenko
Copy link

pjurczenko commented Jun 6, 2021

The current implementation of Cats Effect 2 executor relies solely on Async.async method, which doesn't shift the execution back to the compute pool. This means that operations subsequent to the ElasticSearch queries will be performed on the dispatcher threads used for the non-blocking I/O, which is definitely undesirable. This is how the executor could look like to prevent this:

import cats.syntax.apply._

class CatsEffectExecutor[F[_]: Async: ContextShift] extends Executor[F] {
  override def exec(client: HttpClient, request: ElasticRequest): F[HttpResponse] =
    Async[F].async(cb => client.send(request, cb)) <* ContextShift[F].shift
}

However, such a change would be breaking the binary compatibility.

@sksamuel how do you approach such breaking changes in the elastic4s releases?

@sksamuel
Copy link
Collaborator

sksamuel commented Aug 1, 2021

We can do this for the 8.0 release which will contain other breaking changes.

@stale
Copy link

stale bot commented Oct 1, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Oct 1, 2021
@github-sebastien-boulet
Copy link

This is still relevant

@stale stale bot removed the wontfix label Oct 4, 2021
@igor-vovk
Copy link
Contributor

I think this issue can be closed since it's not relevant since cats-effect 3

@lenguyenthanh
Copy link
Contributor

👍 for close this

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

No branches or pull requests

5 participants