Port blaze-core to cats-effect-3 #3894

yanns · 2020-11-20T07:26:40Z

#3837

It's a WIP.
I'd like to receive some early feedback before attacking the websocket part.

http4s#3837

blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala

blaze-core/src/main/scala/org/http4s/blazecore/util/ChunkWriter.scala

blaze-core/src/main/scala/org/http4s/blazecore/util/package.scala

yanns · 2020-11-20T07:30:30Z

blaze-core/src/test/scala/org/http4s/blazecore/util/CatsEffect.scala

+import scala.concurrent.duration.{Duration, _}
+
+/**
+  * copy of [[cats.effect.testing.specs2.CatsEffect]] adapted to cats-effect 3


We need a migrated https://github.com/djspiewak/cats-effect-testing.

blaze-core/src/test/scala/org/http4s/blazecore/util/Http1WriterSpec.scala

SystemFw

Really not an expert in blaze, but left some comments here and there

blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala

blaze-core/src/main/scala/org/http4s/blazecore/util/ChunkWriter.scala

blaze-core/src/main/scala/org/http4s/blazecore/util/package.scala

yanns · 2020-11-20T20:48:30Z

Thanks for the feedback @SystemFw !

blaze-core/src/main/scala/org/http4s/blazecore/util/Http2Writer.scala

rossabaker · 2020-11-21T04:33:27Z

blaze-core/src/test/scala/org/http4s/blazecore/util/CatsEffect.scala

+
+import scala.concurrent.duration.{Duration, _}
+
+/** copy of [[cats.effect.testing.specs2.CatsEffect]] adapted to cats-effect 3


This would be a good issue on the cats-effect-testing repo.

Until then, I think we could put it in our testing package and maybe use it in multiple modules.

blaze-core/src/test/scala/org/http4s/blazecore/util/Http1WriterSpec.scala

The compilation of the 2 specs seems to wait for the same lock. The compilation never completes. ``` | => org.http4s.blazecore.util.Http1WriterSpec 63s | => org.http4s.blazecore.websocket.Http4sWSStageSpec 63s ```

yanns · 2020-11-21T18:14:42Z

I'm now blocked by the running of the tests: e9eeb95

The 2 specs seems to wait for the same lock. The tests never complete.

yanns · 2020-11-21T18:20:54Z

Update: the issue seems to be only on Http4sWSStageSpec. When I comment Http4sWSStageSpec out, the tests can run. If I comment only Http1WriterSpec out, the tests never complete.

…_ec3

yanns · 2020-11-29T12:35:10Z

blaze-core/src/main/scala/org/http4s/blazecore/websocket/Http4sWSStage.scala

@@ -152,25 +153,26 @@ private[http4s] class Http4sWSStage[F[_]](
        .compile
        .drain

-    unsafeRunAsync(wsStream) {
+    val result = F.attempt(wsStream).flatMap {


Current situation: F.attempt(wsStream) does not make any progress.
In the previous implementation (unsafeRunAsync(wsStream)), wsStream was "shift" to the execution context provided in the constructor.

I'm very far out of the CE3 loop right now. Do we need a start or something similar?

The Dispatcher can start it. See at line 165: D.unsafeRunSync(result).
When I add a timeout at this line, the timeout exception is coming from this place.

unsafeRunSync is blocking, so the semantics have changed here. Try unsafeToFuture instead of unsafeRunSync, or add .start at the end of result

also the attempt can become an handleErrorWith

I still have questions about the .unsafeRunSync(), if this waits for the start.

it waits for the completion of the portion of IO preceding the first async boundary. In this case, because there is a shift immediately, it essentially doesn't wait. (and this is in CE2 anyway)

If yes unsafeRunAsync starts the side-effect on another thread pool right?

Strictly speaking (in CE2), it just follows what the IO does, and will follow whatever async boundary the IO has. In this case there is an immediate shift. In CE3 the model is conceptually simpler.

As unsafeToFuture returns a Future, should we wait for the Future with an Await.result or something similar?

no, we don't need to Await.result, and in fact what you're doing right now with unsafeRunSync is essentially Await.result, which I think it's causing the deadlock in the test.

then we can start the Future and ignore it, just returning Unit.

yes

In fact, call unsafeRunAndForget on Dispatcher

@SystemFw thank you a lot for your detailed answers ❤️

Thank you for porting this :)

ed7c215
932bd3a

so that the tests could run, I had to also do the following: f7d5840

yanns · 2020-11-29T21:08:19Z

current status: all tests are now running.

@rossabaker this PR is quite messy as it contains a lot of commits.
Should I open a new PR with one commit?

SystemFw · 2020-11-29T23:54:52Z

Should I open a new PR with one commit?

The decision is definitely for Ross, but I actually really dislike that. The history is valuable, if you have ever have to run a bisect on a tough bug, big commits are a pain.

In any case, if you do want to do that, there's no need to open a new PR, you can squash on merge (but don't ;))

rossabaker · 2020-11-30T06:40:22Z

I agree with @SystemFw. It also makes it harder to re-review things: we have to look at the whole instead of the recent changes. I usually never force-push a PR unless I don't think anybody has looked at it yet and I think I can tell a better story with a rebase -i.

The more controversial thing is whether we should squash at merge time or not. Squash merges make a cleaner history and we can still click through to see the original intermediate commits. But even then, I think the intermediate commits have value. In addition to bisection, sometimes we branch off WIP to explore a different direction or, like now, maintain parallel branches through big upgrades. Squashing is better for history but worse for collaboration, and we've optimize for the latter.

All that said, I'm excited to see the progress, but it's 1:30am here, so I'll have to review it tomorrow. Hopefully our CE3 experts can keep looking as well.

yanns · 2020-11-30T06:58:12Z

There is one topic I'm feeling unsure about: I had to create one dispatcher per test in Http4sWSStageSpec to that the tests can run (f7d5840)

But I can still share a dispatcher in Http1WriterSpec.

And this is a change I could eventually rollback: c59e3be
Having a timeout when running tests is good not to wait forever. But it also means that slow tests can be flaky in case the timeout is near the execution time.

rossabaker

Is multiple dispatchers a normal usage? I'm a bit worried that having to do that is masking an issue that will prevent the web sockets from working in the real world, but my understanding of CE3 is still a bit hazy.

I like the way the rest of this has shaped up.

rossabaker · 2020-12-04T18:59:21Z

testing/src/main/scala/org/http4s/testing/CatsEffect.scala

+
+/** copy of [cats.effect.testing.specs2.CatsEffect](https://github.com/djspiewak/cats-effect-testing/blob/series%2F0.x/specs2/src/main/scala/cats/effect/testing/specs2/CatsEffect.scala) adapted to cats-effect 3
+  */
+trait CatsEffect {


Has this been contributed upstream? This is fine for now, but it would be good for this to not be ours.

typelevel/cats-effect-testing#117

This reverts commit f7d5840.

yanns · 2020-12-06T11:15:39Z

Is multiple dispatchers a normal usage? I'm a bit worried that having to do that is masking an issue that will prevent the web sockets from working in the real world, but my understanding of CE3 is still a bit hazy.

Thanks to the help from @djspiewak, I could remove blocking calls: ce3ea53

Now all the tests can run with the same Dispatcher. ✌️

For the curious, this is the kind of stack trace I could find by inspecting the threads:

http4s-spec-0 [201] (WAITING)
   jdk.internal.misc.Unsafe.park line: not available [native method]
   java.util.concurrent.locks.LockSupport.park line: 211 
   java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire line: 714 
   java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly line: 1046 
   scala.concurrent.impl.Promise$DefaultPromise.tryAwait0 line: 207 
   scala.concurrent.impl.Promise$DefaultPromise.result line: 225 
   scala.concurrent.Await$.$anonfun$result$1 line: 201 
   scala.concurrent.Await$$$Lambda$12046/0x00000008026c57d8.apply line: not available 
   scala.concurrent.BlockContext$DefaultBlockContext$.blockOn line: 62 
   scala.concurrent.Await$.result line: 124 
   cats.effect.std.DispatcherPlatform.unsafeRunTimed line: 29 
   cats.effect.std.DispatcherPlatform.unsafeRunTimed$ line: 27 
   cats.effect.std.Dispatcher$$anon$1.unsafeRunTimed line: 128 
   cats.effect.std.DispatcherPlatform.unsafeRunSync line: 25 
   cats.effect.std.DispatcherPlatform.unsafeRunSync$ line: 24 
   cats.effect.std.Dispatcher$$anon$1.unsafeRunSync line: 128 
   org.http4s.blazecore.websocket.WSTestHead.<init> line: 40 
   org.http4s.blazecore.websocket.WSTestHead$$anon$1.<init> line: 99 
   org.http4s.blazecore.websocket.WSTestHead$.$anonfun$apply$1 line: 99 
   org.http4s.blazecore.websocket.WSTestHead$$$Lambda$12458/0x0000000802b6dbd0.apply line: not available 
   cats.SemigroupalArityFunctions.$anonfun$map2$1 line: 30 
   cats.SemigroupalArityFunctions$$Lambda$12467/0x0000000802b71ec0.apply line: not available 
   cats.effect.IOFiber.mapK line: 985 
   cats.effect.IOFiber.succeeded line: 804 
   cats.effect.IOFiber.mapK line: 994 
   cats.effect.IOFiber.succeeded line: 804 
   cats.effect.IOFiber.mapK line: 994 
   cats.effect.IOFiber.succeeded line: 804 
   cats.effect.IOFiber.mapK line: 994 
   cats.effect.IOFiber.succeeded line: 804 
   cats.effect.IOFiber.runLoop line: 309 
   cats.effect.IOFiber.execR line: 918 
   cats.effect.IOFiber.run line: 140 
   java.util.concurrent.ThreadPoolExecutor.runWorker line: 1130 
   java.util.concurrent.ThreadPoolExecutor$Worker.run line: 630 
   java.lang.Thread.run line: 832

rossabaker

It was just that unsafeRunSync to create that semaphore in the stage? In CE2, that's a "I promise not to share this" thing. I wouldn't expect it to deadlock things. The Dispatcher is a different world, though.

Anyway, hooray, and great job.

yanns · 2020-12-08T06:58:18Z

It was just that unsafeRunSync to create that semaphore in the stage? In CE2, that's a "I promise not to share this" thing. I wouldn't expect it to deadlock things. The Dispatcher is a different world, though.

Yes, the Dispatcher.unsafeRunSync blocks.
https://github.com/typelevel/cats-effect/blob/349fc13c6a6679ebd71168901874ee332e37ff5b/std/jvm/src/main/scala/cats/effect/std/DispatcherPlatform.scala#L29
Every time we use it, we have to keep it in mind. (and we should better not use it)

Anyway, hooray, and great job.

Thanks, that was a long way.
But I've learned a lot, thanks to all the helpful reviews. ❤️

Port blaze-core to cats-effect-3

Port blaze-core to cats-effect-3

2e788fb

http4s#3837