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

Add IO.unyielding #2633

Closed
wants to merge 3 commits into from
Closed

Conversation

alexandrustana
Copy link
Contributor

@alexandrustana alexandrustana commented Dec 7, 2021

The following PR tries to fix #2610
I'm not really sure what I'm doing here 😅 if this is utter garbage please feel free to close it.

As I've understood from #2478 and #2530, the unyielding method should prevent a section of code from yielding (automatically or manually via cede). The examples from the mentioned issues also show that using start in an unyielding block should prevent fiber scheduling.

Running the following code snippet with the current changes:

(IO(println("foo")) *> IO(println("bar")).start *> IO(println("baz"))).unyielding.unsafeRunAndForget()

generates the following result:

foo
bar
baz

If this is indeed the intended behaviour, my question is if the unyielding block should also affect evalOn or blocking?

@armanbilge
Copy link
Member

armanbilge commented Dec 17, 2021

Thanks for the PR, and all your contributions! Apologies for the delayed response, tbh at least for me personally I still haven't quite wrapped my head around how to make this change. This is an ambitious one 😅 still, here are some comments.

As I've understood from #2478 and #2530, the unyielding method should prevent a section of code from yielding (automatically or manually via cede).

Yes. This is illustrated in #2610 as a sort of "law":

unyielding(cede) <-> unyielding(unit)

Whether it can actually be implemented as a meaningful law, I'm not sure. In any case, we'll eventually want to add unyielding to Concurrent itself I believe, in addition to IO as you've done here.

The examples from the mentioned issues also show that using start in an unyielding block should prevent fiber scheduling.

I could be mistaken, but I don't think that was a goal of unyielding. I don't see why it should affect .start at all to be honest.

I took a brief look at your changes and they seem to be on the right track. But, we'll need a runloop expert to comment on that :) also I think there's some interesting questions here about how to handle the autocede counter during an unyielding block.

@alexandrustana
Copy link
Contributor Author

Thank you @armanbilge for looking over the PR. I'm aware that there is still a lot of improvement to be made to this PR, so any suggestion is more than welcome 😄 (thank you for the Concurrent tip).

I could be mistaken, but I don't think that was a goal of unyielding. I don't see why it should affect .start at all to be honest.

I've thought that using .start was just a way to show that unyielding can eventually end up behaving weirdly if the prevention of shifting is applied to some operators which are used specifically for this scenario.

@armanbilge
Copy link
Member

I've thought that using .start was just a way to show that unyielding can eventually end up behaving weirdly if the prevention of shifting is applied to some operators which are used specifically for this scenario.

Ah, thanks for clarifying! Maybe there is some confusion here, depending on how you define "shifting" 🤔 .start is used to run an iO concurrently and .cede is used for the current fiber to "yield" (give a turn) to other fibers currently in the process.

So if we adjust your example program a bit, I think we can start to see the behavior:

for { // fiber A
  _ <- IO(println("foo")
  _ <- IO(println("bar").start // fiber B
  _ <- IO.cede // fiber A yields, so fiber `B` runs now
  _ <- IO(println("baz")
} yield ()

As written, I think this program should get you foo / bar / baz. But if you wrap it in unyielding it should get you foo / baz / bar. Let me know if that makes sense 😅

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Finally got to this!

Okay so I have two concerns right now. First, I don't like how the name is asymmetrical with cede. But unceding sounds dumb. :-) Second, and more concerningly, I'm worried that people are going to write code which uses this function as more than just a hint. Even some basic examples like cachedRealTime would need this function to behave exactly as implemented on IO in order to function correctly, and that's concerning because we have to make the GenSpawn implementation simply identity. I don't really know how to avoid this issue, though.

@@ -92,6 +92,7 @@ private final class IOFiber[A](

private[this] var canceled: Boolean = false
private[this] var masks: Int = 0
private[this] var unyield: Boolean = false
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this needs to be an Int rather than a Boolean. The problem is that you want IO.unyielding(IO.unyielding(IO.unit) *> IO.cede) to suppress the cede.

@djspiewak
Copy link
Member

Moving this out to v3.5.0 in the interest of being cautious, since we're making a change to the axioms in a meaningful sense.

@djspiewak djspiewak modified the milestones: v3.4.0, v3.5.0 Jun 16, 2022
@alexandrustana
Copy link
Contributor Author

Sorry for being quiet for such a long time.

I'm not sure when I'll be able to pick this up again in the near future, sorry for that.

In the meantime, if someone is interested to pick this up, I'm more than happy to share 😄

@djspiewak
Copy link
Member

It's quite alright! This is a surprisingly thorny one. A lot of the work that needs to be done here is just discussion and analysis. I'm really not convinced that any one direction is better than another so it needs some thought.

@djspiewak
Copy link
Member

So I've spent a few months mulling on this, and while I very much want to add it, I can't see a way of doing so without binary breakage. Or rather, we can add it without binary breakage, but the result will be silent semantic failures in a lot of cases. The whole point of this kind of construct is so that people can rely on it, and the failure modes are very much nondeterministic and difficult to discover.

I think that we push the general version of this back to a future CE4. We could explore a version which is specific to IO, but that has its own unfortunate effects. For the time being, I think it's better to backburner this.

Thank you so much for your work on it, @alexandrustana! I know it probably feels fruitless but honestly this was immensely helpful in exploring the space and trying to figure out what best to do.

@djspiewak djspiewak closed this Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce an unyielding { ... } construct
3 participants