Skip to content

Polysemy concurrency effect #2748

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

Merged
merged 10 commits into from
Oct 5, 2022

Conversation

isovector
Copy link
Contributor

This PR adds a Concurrency effect to polysemy-wire-zoo, which lifts behavior from unliftio for splitting up tasks over multiple threads. In general, this is unsafe in application code and polysemy doesn't handle it very well, and in most cases it is going to create surprising bugs if it's used in application code.

That being said, there are "safe" usages of Concurrency where the only actions being lifted are IO, which is something interpreters can guarantee. The Concurrency effect tracks enforces this safety constraint at the type level.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@isovector isovector temporarily deployed to cachix October 4, 2022 22:35 Inactive
@isovector isovector temporarily deployed to cachix October 4, 2022 22:35 Inactive
@isovector
Copy link
Contributor Author

@mdimjasevic

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I just have one tiny comment in a sequential interpreter, but even if my comment is valid, I think we can do that in a follow-up PR to fix it.

@@ -4,6 +4,9 @@ import Imports
import Polysemy
import Wire.Sem.Concurrency

------------------------------------------------------------------------------

-- | Safely perform "concurrency" by doing it sequentially.
sequentiallyPerformConcurrency :: Sem (Concurrency safe ': r) a -> Sem r a
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have been 'Safe instead of safe in the type :: Sem (Concurrency safe ': r) a -> Sem r a?

Copy link
Contributor

Choose a reason for hiding this comment

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

@isovector , I guess this has slipped your attention because I merged this before you got a chance to reply. Here's just a reminder.

@mdimjasevic mdimjasevic added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 5, 2022
@mdimjasevic mdimjasevic temporarily deployed to cachix October 5, 2022 10:59 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix October 5, 2022 10:59 Inactive
@mdimjasevic
Copy link
Contributor

@isovector , I just pushed an empty commit to your fork/branch to trigger our CI that otherwise has a hard time figuring out it should run. I hope this is fine by you. Otherwise I can copy your branch into wireapp/wire-server and then clone this PR modulo the incoming branch.

@mdimjasevic mdimjasevic merged commit addd15e into wireapp:develop Oct 5, 2022
@isovector
Copy link
Contributor Author

@mdimjasevic thanks for being on top of it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants