Fix releasing resources in TestServerInterpreter #3921
Conversation
| @@ -21,5 +21,5 @@ trait TestServerInterpreter[F[_], +R, OPTIONS, ROUTE] { | |||
| def serverWithStop(routes: NonEmptyList[ROUTE], gracefulShutdownTimeout: Option[FiniteDuration] = None): Resource[IO, (Port, KillSwitch)] | |||
|
|
|||
| def server(routes: NonEmptyList[ROUTE]): Resource[IO, Port] = | |||
| serverWithStop(routes, gracefulShutdownTimeout = None).map(_._1) | |||
There was a problem hiding this comment.
I think the server-release logic should already be part of Resource?
There was a problem hiding this comment.
As far as I understand, when you call .allocated on a Resource it becomes an unsafe Resource[F, (A, IO[Unit])] and it's up to you to call the cleanup function manually.
Scaladoc says:
* If the outer `F` fails or is interrupted, `allocated` guarantees that the finalizers will
* be called. However, if the outer `F` succeeds, it's up to the user to ensure the returned
* `ExitCode => F[Unit]` is called once `A` needs to be released. If the returned `ExitCode =>
* F[Unit]` is not called, the finalizers will not be run.
*
* For this reason, this is an advanced and potentially unsafe api which can cause a resource
* leak if not used correctly, please prefer [[use]] as the standard way of running a
* `Resource` program.
There was a problem hiding this comment.
yes, but there's no allocated here?
There was a problem hiding this comment.
I assumed that serverWithStop always uses allocated, but turns out it does so only for zio-http and http4s.
Also, we shouldn't rely on such assumptions. I can fix this by adjusting serverWithStop for these 2 backends.
However, another issue may be hiding here: tests that do graceful shutdown will manually call the cleanup function before assertions, and then it would be automatically called again after a test finishes? I'll look into that as well, right away.
There was a problem hiding this comment.
I think it should be the responsiblity of serverWithStop implementation to return a proper Resource? Maybe the fix should be over there :) As now we might release resources twice? (once by the original definition, once using the kill switch)
There was a problem hiding this comment.
Maybe we can flip the logic? Make server return a normal Resource that auto-closes, and serverWithStop call server().allocated... so that it always returns a resource that has to be closed manually? I think we even talked about this once but of course I forgot the conclusions ;)
The
serverresource usesserverWithStopand ignores itsreleaseeffect. As a result, non-erroneous closing of this resource doesn't release anything, so we need to call the hook manually.