http4s / blaze

Blazing fast NIO microframework and Http Parser
Apache License 2.0
351 stars 63 forks source link

Redesign tests based on `QueueTestHead` and `onFinalizeWeak` #703

Open RafalSumislawski opened 2 years ago

RafalSumislawski commented 2 years ago

Http1ClientStageSuite and ClientTimeoutSuite."Idle timeout on slow POST body" use a combo of QueueTestHead to mock the network side of the connection and request.body.onFinalizeWeak to trigger some action in QueueTestHead. In these tests onFinalizeWeak is meant to detect when the request has been sent. The problem is that finalisation of the request body stream, does not guarantee the request has been sent. This can be clearly seen in the code of org.http4s.blazecore.util.EntityBodyWriter#writeEntityBody where writeEnd is executed after drain:

  def writeEntityBody(p: EntityBody[F]): F[Boolean] = {
    val writeBody: F[Unit] = p.through(writePipe).compile.drain
    val writeBodyEnd: F[Boolean] = fromFutureNoShift(F.delay(writeEnd(Chunk.empty)))
    writeBody *> writeBodyEnd
  }

This issue leads to flaky tests like this one: https://github.com/http4s/http4s/runs/4498891671?check_suite_focus=true#step:13:5129

RafalSumislawski commented 2 years ago
 ==> X org.http4s.blaze.client.Http1ClientStageSuite.Submit a request line with a query  0.017s munit.ComparisonFailException: /home/runner/work/http4s/http4s/blaze-client/src/test/scala/org/http4s/blaze/client/Http1ClientStageSuite.scala:153 Obtained empty output!
152:      val statusLine = request.split("\r\n").apply(0)
153:      assertEquals(statusLine, "GET " + uri + " HTTP/1.1")
154:      assertEquals(response, "done")
    at munit.Assertions.failComparison(Assertions.scala:297)
    at munit.Assertions.failComparison$(Assertions.scala:287)
    at munit.FunSuite.failComparison(FunSuite.scala:11)
    at munit.Assertions$$anon$1.handle(Assertions.scala:31)
    at munit.internal.difflib.Diffs$.assertNoDiff(Diffs.scala:42)
    at munit.Assertions.$anonfun$assertEquals$1(Assertions.scala:141)
    at get @ fs2.Stream$.$anonfun$interruptWhen$3(Stream.scala:1566)
    at map @ org.http4s.blaze.client.Http1ClientStageSuite.$anonfun$getSubmission$14(Http1ClientStageSuite.scala:124)
    at apply @ org.http4s.blaze.client.Http1ClientStageSuite.$anonfun$getSubmission$12(Http1ClientStageSuite.scala:123)
    at fromFuture @ munit.CatsEffectFunFixtures$ResourceFixture$.$anonfun$apply$8(CatsEffectFunFixtures.scala:59)
    at guarantee @ fs2.internal.CompileScope.$anonfun$interruptWhen$1(CompileScope.scala:384)
    at flatMap @ org.http4s.blaze.client.Http1ClientStageSuite.$anonfun$getSubmission$12(Http1ClientStageSuite.scala:123)
    at apply @ org.http4s.blaze.client.Http1ClientStageSuite.$anonfun$getSubmission$10(Http1ClientStageSuite.scala:122)
    at flatMap @ org.http4s.blaze.client.Http1ClientStageSuite.$anonfun$getSubmission$10(Http1ClientStageSuite.scala:122)
    at void @ org.http4s.blaze.client.PoolManager.$anonfun$createConnection$2(PoolManager.scala:122)

I think this issue may be the root cause of why we have marked so many tests in Http1ClientStageSuite as flaky.

FrancescoSerra commented 2 years ago

Shouldn't this be moved to http4s/blaze ?