zio / zio-http

A next-generation Scala framework for building scalable, correct, and efficient HTTP clients and servers
https://zio.dev/zio-http
Apache License 2.0
787 stars 396 forks source link

StackOverflowError when too many "optional" query parameters defined for Endpoint #2942

Closed Andycharalambous closed 1 month ago

Andycharalambous commented 3 months ago

Describe the bug If an endpoint has 15 or more optional query parameters any attempt to call it will result in a StackOverflowError unless at least one query param value is provided.

To Reproduce Steps to reproduce the behaviour:

type SOIn = ((Option[String],Option[String],Option[String],Option[String],Option[String],Option[String],Option[String],Option[String],Option[String],Option[String]),Option[String],Option[String],Option[String],Option[String],Option[String])
  private val soEndpoint =
    Endpoint(Method.GET / "so")
      .query[Option[String]](query("a").optional)
      .query[Option[String]](query("b").optional)
      .query[Option[String]](query("c").optional)
      .query[Option[String]](query("d").optional)
      .query[Option[String]](query("e").optional)
      .query[Option[String]](query("f").optional)
      .query[Option[String]](query("g").optional)
      .query[Option[String]](query("h").optional)
      .query[Option[String]](query("i").optional)
      .query[Option[String]](query("j").optional)
      .query[Option[String]](query("k").optional)
      .query[Option[String]](query("l").optional)
      .query[Option[String]](query("m").optional)
      .query[Option[String]](query("n").optional)
      .query[Option[String]](query("o").optional)
      .out[String]

private val soHandler: Handler[Any, zio.ZNothing, SOIn, String] = Handler.fromZIO(ZIO.succeed(""))
private val soRoute: Route[Any, Nothing] = soEndpoint.implementHandler(soHandler)

soRoute.run(Request.get("/so"))
Results in:
 java.lang.StackOverflowError
    at zio.http.Handler$$anon$4.apply(Handler.scala:314)

soRoute.run(Request.get("/so?a=test"))
Results in successfull call

Expected behaviour the request should not result in a StackOverflowError

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Smartphone (please complete the following information):

Additional context 3.0.0-RC9

gregor-rayman commented 2 months ago

I have played with this a bit. The problem seems to be in the class Handler.scala method foldCauseHandler (line 308 currently).

Although the method is not really recursive, it delegates to another handler, and there is plenty of them. In my tests, I was getting stack overflow errors when the query had 13 optional parameters. With 12 it run successfully, but there were over 4000 generated handlers.

Here a primitive change to highlight the issue:

final def foldCauseHandler[R1 <: R, Err1, In1 <: In, Out1](
    onFailure: Cause[Err] => Handler[R1, Err1, In1, Out1],
    onSuccess: Out => Handler[R1, Err1, In1, Out1],
  )(implicit trace: Trace): Handler[R1, Err1, In1, Out1] =
    new Handler[R1, Err1, In1, Out1] {
      override def apply(in: In1): ZIO[R1, Err1, Out1] = {
        println(this) // This will flood the console when 13 optional parameters are used, at 14 it fails with stack overflow
        self(in).foldCauseZIO(
          cause => onFailure(cause)(in),
          out => onSuccess(out)(in),
        )
      }
    }
gregor-rayman commented 2 months ago

Endpoint.inplementHanlders generates a hander for every possible input alternative. So every optional query parameter doubles the amount of handlers.

Those handlers are nested, so for 13 optional parameters the stack depth will be 8192.

gregor-rayman commented 2 months ago

To me, the root issue seems to be the encoding of the HttpCodec.optional that encodes the handler as a pair of handlers, one for the parameter present and a fallback handler for the parameter missing. Then all those alternatives are linearized and the resulting handlers are nested. This leads to the huge stack depth.

gregor-rayman commented 2 months ago

I have added an alternative encoding to #2980 that solves the issue.

jdegoes commented 2 months ago

/bounty $350 for a fix that does not change the API.

algora-pbc[bot] commented 2 months ago

💎 $350 bounty • ZIO

Steps to solve:

  1. Start working: Comment /attempt #2942 with your implementation plan
  2. Submit work: Create a pull request including /claim #2942 in the PR body to claim the bounty
  3. Receive payment: 100% of the bounty is received 2-5 days post-reward. Make sure you are eligible for payouts

Thank you for contributing to zio/zio-http!

Add a bountyShare on socials

Attempt Started (GMT+0) Solution
🔴 @gregor-rayman Jul 30, 2024, 5:16:33 PM WIP
🟢 @987Nabil #3000
gregor-rayman commented 2 months ago

/attempt #2942

I see the following possibilities:

1) tweak the .optional method so that it creates only a simple decoder, without a Fallback one or 2) tweak Endpoint.implementHandler so that it does not create a nested structure with 2^n handlers

gregor-rayman commented 2 months ago

In #2980 I have now implemented a fix for the original API query[Option[String]](query("a").optional).

The fix is a bit ugly in the method HttpCodec.optional. It is implemented as a special treatment for query codecs. It does not emit a Fallback codec, but a query codec that does not throw an exception when the query parameter is missing.

The nesting of handlers in Endpoint.implementHandler is still not fixed, but the 2^n issue caused by .optional creating a Fallback is. (for queries only - the 2^n issue exists for the headers and all other .optional codecs).

I have still left the Query.queryOptional and Query.queryAllOptional in the source code. queryOptional("a") has the same semantics as query("a").optional, so perhaps it can be removed.

queryAllOptional however does not return an Option[Chunk[_]], but just Chunk[_] that can be empty. queryAll("a") before would fail if the request omits the query parameter a, and return an empty chunk, if the request had the parameter without a value (like: ?a instead of ?a=). Now it will never return an empty Chunk and it will fail for ?a. queryAllOptional will not fail, it will return an empty chunk both if the query parameter was not present, or was present without value.

algora-pbc[bot] commented 2 months ago

💡 @987Nabil submitted a pull request that claims the bounty. You can visit your bounty board to reward.

algora-pbc[bot] commented 2 months ago

@gregor-rayman: Reminder that in 7 days the bounty will become up for grabs, so please submit a pull request before then 🙏

algora-pbc[bot] commented 1 month ago

🎉🎈 @987Nabil has been awarded $350! 🎈🎊