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

Remove IOLocal#scope, revert #3214 #3405

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

armanbilge
Copy link
Member

Reverts #3214, with apologies to @iRevive. IIRC I even encouraged that PR, so my bad.

Basically, for scope to work correctly, the acquire and release effects must run on the same fiber. Resource cannot guarantee this, so IMO it's a leaky abstraction. Furthermore Ross and I demonstrated various weird/surprising behaviors when mixed with other combinators starting from #3360 (comment).

So yeah, I feel very unsure about exposing an API like this now. If we want to offer "higher-level" APIs that can be used safely, my vote is for something like #3385.

@rossabaker
Copy link
Member

I have incidentally talked myself into it being fine. There are pointy bits, but I think the behavior is correct according to spec, and there are lots of other pointy bits. Why not remove .set, which also does weird shit in races?

I agree that higher-level APIs are needed for safe use. #3385 is excellent for "static monadic regions", but not up to the task of "dynamic monadic regions", to borrow terms from #634. I'm still pursuing that holy grail for otel4s.

@armanbilge
Copy link
Member Author

I think the behavior is correct according to spec

Is it? It says this.

The original value is restored upon the finalization of a resource.

There's no way it can guarantee this, without guaranteeing that it can run those on the same fiber. Which it cannot, because allocated.

@rossabaker
Copy link
Member

Okay, that's fair. But allocated is "an advanced and potentially unsafe api which can cause a resource leak if not used correctly". Is there an adversarial function we can pass to use that would break it?

Stream gets into trouble not by calling allocated, but pattern matching to destructure Resource.Allocated, which I consider morally the same thing.

@armanbilge
Copy link
Member Author

armanbilge commented Feb 5, 2023

But allocated is "an advanced and potentially unsafe api which can cause a resource leak if not used correctly"

Yes, but whose job is it to use correctly? If I compose my local.scope resource with some other resources and hand it off to something that does .allocated (e.g. a pool), is it their fault that it all went wrong?

@armanbilge
Copy link
Member Author

The problem is, that even if allocated is an advanced API, you can use it correctly even if you run the acquire and release on different fibers. That's allowed.

Which is why this API doesn't work: to deliver what it promises, it relies on behavior which is not guaranteed.

@rossabaker
Copy link
Member

That's fair. FS2 is using Allocated appropriately: it releases what it acquires. We are getting bad behavior with FS2 and IOLocal not because FS2 is buggy, but because IOLocal imposes constraints FS2 isn't designed for.

@armanbilge armanbilge added this to the v3.5.0 milestone Feb 6, 2023
@djspiewak
Copy link
Member

I think there are enough footguns here that I'd rather err on the side of caution. We can always bring it back in a later release.

@djspiewak djspiewak merged commit 5f7b243 into typelevel:series/3.x Feb 6, 2023
@wunderk1nd-e
Copy link

@djspiewak @rossabaker Would going back to the originally proposed implementation using bracket dodge the issues with how the Resource might end up getting used?
#3128

@armanbilge
Copy link
Member Author

@wunderk1nd-e thanks for chiming in! Yes, I believe that would address the issues, since the acquire and release phases of bracket are guaranteed to run on the same fiber.

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

Successfully merging this pull request may close these issues.

4 participants