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

[RFC] feat: scoped requests #3229

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jgranstrom
Copy link
Contributor

@jgranstrom jgranstrom commented Dec 3, 2024

/fixes #3197
/claim #3197

RFC Adds support for per-request Scopes at server level.

Adds the ability to run a server that provides a scope for each request. This is to not add any overhead or overly complicate the existing path that does not provide request-bound scopes.

I have not added tests or similar yet as I need some feedback on the idea of this as a solution first.
(Edit: also only implemented with the Netty driver for the above reason)

Copy link

algora-pbc bot commented Dec 3, 2024

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe.

@jgranstrom jgranstrom force-pushed the feat-scoped-requests branch from 375bc68 to a8df79e Compare December 3, 2024 16:26
@jgranstrom jgranstrom changed the title feat: scoped requests [RFC] feat: scoped requests Dec 3, 2024
@jgranstrom jgranstrom force-pushed the feat-scoped-requests branch from a8df79e to 87329b7 Compare December 4, 2024 06:12
handler.apply
case Right(scopedHandler) =>
val handler = scopedHandler.toHandler
(req: Request) => ZIO.scoped(handler(req))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guersam
Copy link
Contributor

guersam commented Dec 9, 2024

Thanks for tackling this issue! My two cents:

I think the Scope env could be resolved by the handler via something like Handler.scoped. It reflects the locality of each scope more precisely by not leaking the requirement, and it's more consistent with ZLayer.scoped.

@jgranstrom
Copy link
Contributor Author

Thanks for tackling this issue! My two cents:

I think the Scope env could be resolved by the handler via something like Handler.scoped. It reflects the locality of each scope more precisely by not leaking the requirement, and it's more consistent with ZLayer.scoped.

Yeah I went with that initially but it gets fairly convoluted to thread that through everything, so I thought of a less invasive option to just fork it top-level, but also agree that it would be cleaner for the outside to do that.

@jgranstrom
Copy link
Contributor Author

jgranstrom commented Jan 5, 2025

Thanks for tackling this issue! My two cents:

I think the Scope env could be resolved by the handler via something like Handler.scoped. It reflects the locality of each scope more precisely by not leaking the requirement, and it's more consistent with ZLayer.scoped.

I went over this one more time, with that approach. And I landed on what is originally in this PR being the most sensible approach to me still. It's a balance between introducing overhead everywhere to determine if a Scope is needed, vs just always creating the Scope.

For something like Handler.scoped it's not known until routes are traversed and matched whether a Scope is needed, meaning it must be created lazily when invoking the handler. However, that Scope must then leak outside of the Handler so that it can be held open and later closed by the Driver when the request-response fully completes.

Server.installScoped could be named better, or the syntax could be changed to something like Server.install(app.scoped), or some variation that is more proper.

However the option of Handler.scoped is really messy. It must compose correctly, all over the place, to retain it being Scoped with other handlers, and also onto Routes. Also applying Handlers must then be able to receive a Scope pre-apply, that is lazily created by the Driver, that is also not closed as part of running the handler itself, but by the Driver. I think that is just too much complexity, and most likely overhead, versus what I proposed here.

It also fits other scoped operators fairly well still, if moving it to Server.scoped.install(routes), as that similarly to ZIO.scoped etc allows Routes to have Scope in R, and erases it by providing it via the server. That seems reasonably ZIO-y IMO? I just threw it on installScoped for the RFC.

WDYT? @guersam @987Nabil 🙏

Edit: Also with this I was thinking to have a completely separate inbound handler when it should provide a scope. Since I must add some overhead in it to manage the scope lifetime that is not needed otherwise. That also becomes fairly straight-forward when it is decided at server-level to provide request-Scopes or not.

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