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

CatsResourceIO in scalatest doesn't release the resource #300

Open
ghostbuster91 opened this issue Apr 3, 2022 · 8 comments · May be fixed by #555
Open

CatsResourceIO in scalatest doesn't release the resource #300

ghostbuster91 opened this issue Apr 3, 2022 · 8 comments · May be fixed by #555

Comments

@ghostbuster91
Copy link

Info:

cats-effect-testing-scalatest" % "1.4.0" 
openjdk 11.0.12

Given following example:

import cats.effect._
import cats.effect.testing.scalatest.{AsyncIOSpec, CatsResourceIO}
import org.scalatest.freespec.FixtureAsyncFreeSpec

class TestExample extends FixtureAsyncFreeSpec with AsyncIOSpec with CatsResourceIO[Int] {
  val resource: Resource[IO, Int] = Resource.make(IO.println("acquired").as(1))(_ => IO.println("released"))

  "should close resource" in { outer =>
    val inner = Resource.make(IO.println("acquired inner"))(_ => IO.println("released inner"))
    inner
      .use { _ =>
        IO.println(outer).as(succeed)
      }
  }
}

I would expect scalatest to release the fixture resource. Instead the resource is not being released.

I think that there are two problems with the current solution.

First, we schedule the shutdown action here: https://github.com/typelevel/cats-effect-testing/blob/series/1.x/scalatest/shared/src/main/scala/cats/effect/testing/scalatest/CatsResource.scala#L76 which is asynchronous and then we immediately clear the shutdown variable. This clear action has a potential to be executed before the scheduled future is run resulting in the release code not being evaluated.

However, only rewriting that to something like:

  override def afterAll(): Unit = {
    UnsafeRun[F].unsafeToFuture(
      shutdown >> Sync[F] delay {
        gate = None
        value = None
        shutdown = ().pure[F]
      },
      finiteResourceTimeout
    )
  }

Doesn't fix the problem.

The second part is somehow related to the execution context used by scalatest.

I didn't dig deep enough into the scalatest codebase to prove that but it seems to me that the execution context gets closed(?) as soon as the test suite completes.

Changing the EC used to evaluate future actions together with previous solution fixes the problem.

  private lazy val _ResourceUnsafeRun =
    new UnsafeRun[IO] {
      private implicit val runtime: IORuntime = createIORuntime(ExecutionContext.global) //here using global instead of inherited exectionContext

      override def unsafeToFuture[B](ioa: IO[B]): Future[B] =
        unsafeToFuture(ioa, None)

      override def unsafeToFuture[B](ioa: IO[B], timeout: Option[FiniteDuration]): Future[B] =
        timeout.fold(ioa)(ioa.timeout).unsafeToFuture()
    }

I am not saying that this is the correct solution, I am only including that to give more information about the underlying issue.

While trying to understand that issue I also came across these two issues which I think might be relevant:

@froth
Copy link

froth commented May 2, 2022

I am also running into this problem.

(here was some thought which does not make sense any longer since I read the scalatest issues you linked. But I am unable to delete this comment)

@froth
Copy link

froth commented May 2, 2022

The following workaround solves the problem for me (jvm):

class TestExample extends FixtureAsyncFreeSpec with AsyncIOSpec with CatsResourceIO[Int] {

override val executionContext: ExecutionContext = ExecutionContext.global
// ...
}

@ghostbuster91
Copy link
Author

I think that changing only the execution context to the one which is backed by multiple threads can result in some concurrency issues as described in the first part.

@sentenza
Copy link

sentenza commented May 9, 2023

The following workaround solves the problem for me (jvm):

class TestExample extends FixtureAsyncFreeSpec with AsyncIOSpec with CatsResourceIO[Int] {

override val executionContext: ExecutionContext = ExecutionContext.global
// ...
}

The only workaround that is actually working for me is using the global EC. Otherwise the resource defined here will never be deallocated/released:

class RpcLocationServiceSpec
    extends FixtureAsyncWordSpec
    with AsyncIOSpec
    with Matchers
    with OptionValues
    with AsyncMockFactory
    with CatsResourceIO[IO[OutcomeIO[Nothing]]] {
  abstract class MockUnLocationService(
      unLocationRepo: UnLocationRepositoryAlgebra[IO],
      logger: SelfAwareStructuredLogger[IO]
  ) extends UnLocationService[IO](unLocationRepo, logger)
  implicit val unsafeLogger: SelfAwareStructuredLogger[IO] = Slf4jLogger.getLogger[IO]
  private val testRpcPort: Int                             = 9095
  private val mockLocationService                          = mock[MockUnLocationService]
  private val rpcConfig                                    = RpcConfig("127.0.0.1", testRpcPort)
// THE ONLY WORKAROUND THAT WORKS:
  override val executionContext: ExecutionContext = ExecutionContext.global
  override val resource: Resource[IO, IO[OutcomeIO[Nothing]]] =
    RpcLocationService
      .service[IO](mockLocationService, unsafeLogger)
      .flatMap(RpcServerManager.startServer[IO](_, rpcConfig, unsafeLogger).useForever.background)

@adamretter
Copy link

We are also experiencing this problem with the simple reproducible code:

import cats.effect.{IO, Resource}
import cats.effect.testing.scalatest.{AsyncIOSpec, CatsResourceIO}
import org.scalatest.wordspec.FixtureAsyncWordSpec

class TestMeSpec extends FixtureAsyncWordSpec with AsyncIOSpec with CatsResourceIO[Int] {

  override val resource: Resource[IO, Int] = Resource.make(IO.pure(1).flatTap(id => IO.println(s"Acquired Resource: $id")))(id => IO.println(s"Released Resource: $id"))

  "cats resource specifications" should {
    "should acquire and release the resource" in { id =>
        IO.print(s"I have the resource: $id")
    }
  }
}

This only prints:

Acquired Resource: 1
I have the resource: 1[info] TestMeSpec:
[info] cats resource specifications
[info] - should should acquire and release the resource

Whereas we expect it to instead print:

Acquired Resource: 1
I have the resource: 1[info] TestMeSpec:
[info] cats resource specifications
[info] - should should acquire and release the resource
Released Resource: 1

@sentenza
Copy link

@djspiewak @sh0hei do you know who could look into this issue? It's been reported more than 1 year ago 🙏

@Daenyth
Copy link

Daenyth commented May 16, 2023

I took a glance at CatsResource and it seems fine - I think the problem here is the ExecutionContext that the futures are running on being terminated before the shutdown step can complete. I'm not 100% certain though

@AdamJKing
Copy link

I believe this is happening due to an intentional design choice with ScalaTest's SerialExecutionContext which only executes Futures scheduled during the test body. This execution context only executes scheduled futures when the runNow method is explicitly invoked;

https://github.com/scalatest/scalatest/blob/6bf2e09b646909547ccbd64480a2cf1089e7f8dc/jvm/core/src/main/scala/org/scalatest/concurrent/SerialExecutionContext.scala#L111

For async test suites this is performed as part of the deferred outcome / status that occurs when the test is run;

https://github.com/scalatest/scalatest/blob/6bf2e09b646909547ccbd64480a2cf1089e7f8dc/jvm/core/src/main/scala/org/scalatest/AsyncEngine.scala#L377C19-L377C19

However the BeforeAndAfterAll trait registers the teardown action as happening after the test body has executed and returned a status;

https://github.com/scalatest/scalatest/blob/6bf2e09b646909547ccbd64480a2cf1089e7f8dc/jvm/core/src/main/scala/org/scalatest/BeforeAndAfterAll.scala#L223-L226

Since the runNow has already completed any new tasks on the queue won't be executed

Using a different EC works because task execution is no longer dependant on being triggered during the test. Would there be any issues using a dedicated EC for the resources while still having IO in test delegated to the serial execution context?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants