-
Notifications
You must be signed in to change notification settings - Fork 416
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
Provide a Scope
per server call (#3197)
#3276
base: main
Are you sure you want to change the base?
Conversation
e7140d6
to
631581e
Compare
zio-http/jvm/src/main/scala/zio/http/netty/server/ServerInboundHandler.scala
Outdated
Show resolved
Hide resolved
6d31d72
to
a5d85d1
Compare
@@ -98,7 +98,7 @@ final case class TestServer(driver: Driver, bindPort: Int) extends Server { | |||
_ <- driver.addApp(provided, r) | |||
} yield () | |||
|
|||
override def install[R](routes: Routes[R, Response])(implicit | |||
override def install[R](routes: Routes[Scope & R, Response])(implicit |
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.
What's the purpose of this Scope
requirement?
I'd prefer hiding it from the Routes
because it makes users to provide Scope
that is not actually used.
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.
The Scope has to be here, so that it is "eaten" up by the method. The effect that is returned has no more requirement of scope. That means, it is not possible to use a Scope that extends over a single request from inside a request. That is intentionally
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.
I see. I misinterpreted the code due to this line.
I also wondered if we could do the elimination earlier, RoutePattern#->
, Routes.apply
or etc. for example, but it could be error prone and let the compiler do more work.
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.
No, the routes is the right place. We need to close the Scope in the driver, after netty write is over.
If we eliminate the Scope earlier, we would need to close the scope earlier too.
Else you do route.run(..) and your scope is not closed/provided. That would be a bug
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.
By 'eliminating', I mean something like this:
trait Route[-Env, +Err] {
def apply(request: Request): ZIO[Scope & Env, Response, Response] = ???
}
case class RoutePattern[A](...) {
def ->[Env, Err, I](handler: Handler[Scope & Env, Err, I, Response]): Route[Env, Err] = ???
}
Unlike the other environments, Scope
is a local requirement per request. I think this encoding reveals the relationship more clearly, but there's two downsides that we need to modify every route forming function including Endpoint DSL and the burden to the compiler of the added type calculation per route . In this respect, I think the current design in this PR is still good.
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.
@guersam I tried that out. It complicates the code, has negative impact on type inference and all just to save Scope
on the type of Routes
seems like a bad trade off
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.
a5d85d1
to
3c473b1
Compare
3c473b1
to
09f624a
Compare
FWIW I tested this on the real world case of #3197 and it looks good. |
fixes #3197
/claim #3197