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
FiberRef (#618) #665
FiberRef (#618) #665
Conversation
Is there anything I can do in order to help with this? Maybe prepare some sort of spec? |
@hanny24 Working on a spec / unit test suite for We want to ensure that:
Right now these tests for FiberLocal will fail, but they can inform implementation of the work. |
I can share more ideas over the weekend. |
My main idea for an API now is: object ZIO {
def inheritLocals(that: Fiber[_, _]): UIO[Unit]
} This would be called automatically in Locals would be stored per fiber in a weak map: This involves internalizing FiberLocal into the runtime system. We can probably improve the API as a result, making an initial value mandatory, for example, so that getting a fiber local always succeeds. |
@hanny24 ^^^ |
Seems pretty straight-forward to me, however I have two notes:
I'm not sure just yet how this is related. Nevertheless, I'll try to implement your proposal over the weekend. |
Hmm, after some deeper thought it's not that clear anymore. I'm not really sure about
What should cc @jdegoes |
@hanny24 Let me take a closer look at this and get back... apologies for the delay. |
@jdegoes ping: Any thoughts on this? |
@hanny24 I'm back and read to help push this through to completion! So I think we need the following ingredients:
Please let me know if anything is unclear or if this doesn't come together as I hope. 😄 |
Hi, Does my comment make any sense to you? |
@hanny24 You are right. We should put the method on Fiber, not add a new standalone op. Then it will be perfectly clean and well-defined. |
@hanny24 You are right. We should put the method on Fiber, not add a new op. Then it will be perfectly clean and well-defined. |
Maybe I have one more question related to for {
local <- FiberLocal.create(42)
fiber1 <- doSomething(local).fork
fiber2 <- doSomethingElse(local).fork
fiber = fiber1.orElse(fiber2)
_ <- fiber.join
result <- local.get
} yield result Assuming that both The best thing I can come up with is to accept |
Oh, I just had another idea: we could make |
|
On a second thought this doesn't seems as really clear. How can we update What would make sense to me is to define FiberLocal as |
Do you see any problems with that approach? |
OK, I'll try to describe the issue I see better:) I see a problem with this:
I can create a new Is there anything I missed in your proposal? I don't see how newly created |
# Conflicts: # core/shared/src/main/scala/scalaz/zio/ZIO.scala # core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
You are right! I think your proposal will work:
This way there is a single identity for the fiber local, even as it crosses fork boundaries, and it will be garbage collected only in the event it is not reachable from anywhere. |
@hanny24 Let me know if you need any more help. I think we're getting close to something really nice! Relatedly, I think we should "modernize" the |
|
9c847d6
to
99fbc89
Compare
# Conflicts: # core/shared/src/main/scala/scalaz/zio/ZIO.scala # core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
I tried to implement this. However it turned out little bit different. I'll try to describes the changes I have done below. The main issue is that some fiber can access Also, in current implementation child fiber does not see any updates done by a parent. In order to implement this we can either:
I kind of like the second approach as you only pay for what you need. Any thoughts on this? |
Great! I will inspect the logic in one last review when you are ready.
Great catch! |
* otherwise it returns a default value. | ||
* This is a more powerful version of `updateSome`. | ||
*/ | ||
final def modifySome[B](default: B)(pf: PartialFunction[A, (B, A)]): UIO[B] = modify { v => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love all these new methods, and by making them consistent, we're helping to make sure users don't have to re-learn a similar set of primitives for each new type of references (Ref
, RefM
, TRef
, and FiberRef
).
core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
Outdated
Show resolved
Hide resolved
core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
Outdated
Show resolved
Hide resolved
core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments and then looks good to merge! 🎉
I consider this ready to be merged. I implemented all your comments and I also added |
core/shared/src/main/scala/scalaz/zio/internal/FiberContext.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work! I saw that we are not using FiberId
in the Map
, and I suggest we delete that. Otherwise, looks ready to merge! 🎉
I'm glad to help! |
This is my initial take on #618. It's still very much in progress, contains no user documentation, the code itself is not in the best possible shape...nevertheless I would like you to review my approach. I'm not really sure if I have not overseen something as I'm not very familiar with ZIO code base.
What do you think about it? Do you see it as a way how #618 can be implemented? If we agree that this is the way, I'm more that happy to implement this for real.