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 a batched API to ZClient #3008

Merged
merged 14 commits into from
Aug 27, 2024
Merged

Conversation

kyri-petrou
Copy link
Collaborator

@kyri-petrou kyri-petrou commented Aug 7, 2024

/fixes #2956
/fixes #2957
/claim #2956
/claim #2957

In a nutshell, this PR adds the following methods and updates docs/examples/tests to change most usages of Client.request to these new methods. The first 3 will materialize the body in memory, whereas the latter will embed the Scope into the ZStream, which will be closed when the stream ends.

object Client {
  def quick(request: Request): RIO[Client, Response]

  def quickWith[A](request: Request)(f: Response => A): RIO[Client, A]

  def quickWithZIO[R, A](request: Request)(f: Response => RIO[R, A]): RIO[Client & R, A]

  def quickWithStream[R, A](request: Request)(f: Response => ZStream[R, Throwable, A]): ZStream[Client & R, Throwable, A]
}

@987Nabil @jdegoes, I'm REALLY tempted to also change the get, post, put, patch, delete and head methods on Client to also materialize the body in memory. The issue with this though is that if there are users using these methods to stream responses, then we'll be messing things up for them.

Wdut? Should we change those methods as well and document it very clearly in the release notes, or leave them as is and hope for the best that users are handling the Scope properly?

@987Nabil
Copy link
Contributor

987Nabil commented Aug 7, 2024

@kyri-petrou then you would add explicit streaming methods?

@kyri-petrou
Copy link
Collaborator Author

kyri-petrou commented Aug 7, 2024

@kyri-petrou then you would add explicit streaming methods?

@987Nabil You mean adding add methods for getStreaming, putStreaming etc?

@987Nabil
Copy link
Contributor

987Nabil commented Aug 9, 2024

Yes. Make the quick the default and add streaming variants. Do you think it makes sense? But if we keep the quick I'd try to find another name.

@kyri-petrou kyri-petrou changed the title Quick client methods Add a simple API to ZClient Aug 11, 2024
@kyri-petrou
Copy link
Collaborator Author

@987Nabil I ended up going down a little different path in the end. Instead of re-implementing all of the methods to have variants that don't require a Scope, instead I added the simple method that exposes the HTTP request methods of the Client but without the Scope. Example usages:

client.simple.get("/foo")
client.simple(Request.get(???))
client.simple.request(Request.get(???))

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 59.45946% with 15 lines in your changes missing coverage. Please review.

Project coverage is 65.62%. Comparing base (e2f2cb5) to head (d3a2bbe).
Report is 1 commits behind head on main.

Files Patch % Lines
...-http/shared/src/main/scala/zio/http/ZClient.scala 53.33% 14 Missing ⚠️
...shared/src/main/scala/zio/http/ZClientAspect.scala 75.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3008      +/-   ##
==========================================
- Coverage   65.65%   65.62%   -0.03%     
==========================================
  Files         155      155              
  Lines       10134    10142       +8     
  Branches     1899     1909      +10     
==========================================
+ Hits         6653     6656       +3     
- Misses       3481     3486       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@987Nabil
Copy link
Contributor

@kyri-petrou I thought this is not working for streaming bodies, when we don't handover a callback or?

@kyri-petrou
Copy link
Collaborator Author

kyri-petrou commented Aug 11, 2024

@987Nabil it is working for streaming bodies. What happens with streaming bodies is that the request body will be materialized in memory. Basically these two are equivalent when we have a streaming body:

client.simple.get("/foo").flatMap(_.body.asString)

ZIO.scoped(client.get("/foo").flatMap(_.body.asString))

The only usage where something is different is this:

ZStream.unwrap(client.simple.get("/foo").map(_.body.asStream))

ZStream.unwrapScoped(client.get("/foo").map(_.body.asStream))

In this case, the resulting stream from the simple API will not actually stream data as it arrives, it is just a Chunk[Byte] already materialized into memory lifted into a ZStream

@987Nabil
Copy link
Contributor

@kyri-petrou idk if this is not a problem. You don't necessarily know, if the server returns a chunked stream. Or if it will fit in memory. I see the bug report already coming 🙈

@kyri-petrou
Copy link
Collaborator Author

@kyri-petrou idk if this is not a problem. You don't necessarily know, if the server returns a chunked stream. Or if it will fit in memory. I see the bug report already coming 🙈

But this would still be a problem if they don't explicitly stream the response, even with the current API. And it's only a problem if a user decides to use the simple API, the "standard" one behaves the same way as before. There is also a stream method on Client now, so users can use that one if they want to stream the response and not worry about the Scope.

The only way to make this can be made more safe is for the simple API to return a supertype of Response where the body cannot be streamed.

I documented the caveat of the simple API both in the documentation and in the scaladoc. I think that if after documenting it very clearly users still end up using it to stream responses, experience said bug, again not read the scaladoc, and end up filing an issue then at this point it's a bit up to them.

@987Nabil
Copy link
Contributor

@kyri-petrou I am not sure about simple as a name I'll give it some thought

@jdegoes
Copy link
Member

jdegoes commented Aug 16, 2024

@kyri-petrou Here's an idea I just want to float for purposes of discussion -- I am not 100% convinced, but it has some promising elements.

Let's say Client has a type parameter that represents "environmental dependencies added during request execution". Then Client[Any] becomes the type of a client that adds no environmental dependencies during request execution. However, Client[Scope] becomes the type of a client that adds a dependency on Scope during request execution.

Then a streaming client must clearly be Client[Scope], but a batched client can be Client[Any], and, moreover, there can exist a single unary operator on Client that can batch up streaming responses, allowing taking a Client[Scope] into a Client[Any] (.batched?). Finally, choice of default constructors should be re-evaluated, and possibly, batched is the default, with streaming being opt-in for those use cases where it makes sense.

What have we achieved in this design?

  • No more methods nor duplication of any methods
  • Safe (loss-less) default behavior, with streaming as an opt-in
  • Identical API to what we have today, with no need to learn anything new
  • Ability to abstract over alternate client implementations that might impose other requirements on a per request basis, e.g. Auth (not that such would be part of this PR)

@jdegoes
Copy link
Member

jdegoes commented Aug 16, 2024

One more comment, only partially related to this PR, and mostly just for purposes of future PRs: essentially, for purposes of best-practice architecture, I think all functionality should be directly exposed on trait/class. This means no functionality can be implemented only in terms of R (such as deprecated accessor pattern). Because as convenient as that is, sometimes, it encourages people to go directly to such "accessors", then makes it difficult to do performant and clean layer composition (in a pure layer architector, R is used only for scope, or other locally eliminable context; never for dependency management).

# Conflicts:
#	zio-http-example/src/main/scala/example/ServerSentEventAsJsonEndpoint.scala
#	zio-http-example/src/main/scala/example/ServerSentEventEndpoint.scala
#	zio-http-example/src/main/scala/example/endpoint/CliExamples.scala
#	zio-http/shared/src/main/scala-2/zio/http/UrlInterpolator.scala
@kyri-petrou kyri-petrou changed the title Add a simple API to ZClient Add a batched API to ZClient Aug 17, 2024
@kyri-petrou
Copy link
Collaborator Author

@jdegoes I reimplemented the client so that now there is an S >: Scope bound on it, and when it's Any the batched API erases the Scope dependency. The stream and socket methods are only available when S =:= Scope to make it more type-safe.

In line with your last comment, I'm considering is to remove the -Env bound on the Client altogether (also from BodyEncoder, BodyDecoder and Driver. However I didn't do this as I wanted to first see if you agree with this approach and hear your thoughts

@987Nabil
Copy link
Contributor

987Nabil commented Aug 17, 2024

@kyri-petrou I am not sure, if we should remove env. Right now, it is buried deep in the lib and average joe would never touch it. But I could imagine, that it could enable some future features. Since the user facing API is only the returned env that by default is always Any, I'd tend to leaving it in.


final case class ZClient[-Env, -In, +Err, +Out](
final case class ZClient[-Env, S >: Scope, -In, +Err, +Out](
Copy link
Member

Choose a reason for hiding this comment

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

  1. In general, I am not a fan of constraining type parameters at the declaration site, unless absolutely strictly required. The constraints add a lot of work for no real benefit and also interfere with evolution.
  2. Because this S is added to each request response (covariant), and there it appears in contravariant position, we should be able to make the parameter contravariant, unless I'm missing something.

This would change the signature like so:

Suggested change
final case class ZClient[-Env, S >: Scope, -In, +Err, +Out](
final case class ZClient[-Env, -S, -In, +Err, +Out](

In which case I might rename:

Suggested change
final case class ZClient[-Env, S >: Scope, -In, +Err, +Out](
final case class ZClient[-Env, -ReqEnv, -In, +Err, +Out](

"The env added to requests."

Or something similar (AddEnv).

Copy link
Collaborator Author

@kyri-petrou kyri-petrou Aug 25, 2024

Choose a reason for hiding this comment

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

I am not a fan of constraining type parameters at the declaration site

Main reason that I went with the constraining type is that removing it doesn't affect binary compatibility, so I thought perhaps it's better to be more strict and remove it if needed. But I agree I don't think that it adds much benefit so I'll remove it

we should be able to make the parameter contravariant, unless I'm missing something

@jdegoes the problem with making it contravariant is that then this becomes possible:

  val batchedClient: ZClient[Any, Any, Body, Throwable, Response] = ???
  val streamingClient: ZClient[Any, Scope, Body, Throwable, Response] = batchedClient

This is a problem because once we've eliminated the Scope the client cannot stream thereafter. The only way to enforce it AFAICT is via an invariant type param


```scala
object Client {
def batched(request: Request): ZIO[Client, Throwable, Response] = ???
Copy link
Member

Choose a reason for hiding this comment

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

Thinking more about this (good) naming convention: could it make sense to rename Client.request to Client.streaming?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since this is a widely used method, I ended up keeping Client.request and annotated it deprecated so that we can point users to the new methods without breaking their code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the deprecated. Then we have to keep this method in until 4.x
I think we should remove it and write migration docs

Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

Just a couple comments, but looks great! Beautiful API.

# Conflicts:
#	zio-http-example/src/main/scala/example/endpoint/CliExamples.scala
@kyri-petrou
Copy link
Collaborator Author

@jdegoes I applied all your recommendations minus making the ReqEnv contravariant (see #3008 (comment)).

I also updated the docs to first introduce the "streaming" client (i.e., default) followed up by the "batched" client/methods. I hope this will be clearer for users to follow

@jdegoes jdegoes merged commit 8bd449c into zio:main Aug 27, 2024
33 of 34 checks passed
@kyri-petrou kyri-petrou deleted the quick-client-methods branch August 31, 2024 06:09
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.

Improve ZClient UX with regards to Scope ZClient memory leaks finalizers on each request
4 participants