MaxTotal does not take idle resources into account

[Jasper-M]

scala> val p = IO.ref(0).flatMap(ref =>
     |   KeyPool.Builder((i: Int) => Resource.make(ref.update(_ + 1).as(i))(i => IO.println(s"kill $i") *> ref.update(_ - 1)))
     |     .withMaxTotal(5)
     |     .withMaxIdle(5)
     |     .build.use{ keypool =>
     |       keypool.take(0).replicateA(5).use(_ => ref.get.flatMap(i => IO.println(s"total $i"))) *>
     |       keypool.take(1).replicateA(5).use(_ => ref.get.flatMap(i => IO.println(s"total $i"))) *>
     |       IO.println("DONE") *>
     |       ref.get
     |     }
     | )
val p: cats.effect.IO[Int] = IO(...)

scala> p.unsafeRunSync()
total 5
total 10
kill 1
kill 1
kill 1
kill 1
DONE
kill 0
kill 0
kill 0
kill 0
kill 0
kill 1
val res29: Int = 6

As you can see, at one point 10 resources are allocated, while the max is set to 5. At the end there are still 6 which might be a separate issue...

[rossabaker]

I think the problem is we're releasing the permit (acquired line 299) after returning items to the pool (line 307), not when the connection is dropped from the pool.

keypool/core/src/main/scala/org/typelevel/keypool/KeyPool.scala

Lines 298 to 312 in 41c4f51

	for {
	_ <- kp.kpMaxTotalSem.permit
	optR <- Resource.eval(kp.kpVar.modify(go))
	releasedState <- Resource.eval(Ref[F].of[Reusable](kp.kpDefaultReuseState))
	resource <- Resource.make(optR.fold(kp.kpRes(k).allocated)(r => Applicative[F].pure(r))) {
	resource =>
	for {
	reusable <- releasedState.get
	out <- reusable match {
	case Reusable.Reuse => put(kp, k, resource._1, resource._2).attempt.void
	case Reusable.DontReuse => resource._2.attempt.void
	}
	} yield out
	}
	} yield new Managed(resource._1, optR.isDefined, releasedState)

Note that the original definition of maxTotal, through 0.4.7, allowed unlimited allocations. There was discussion of the new semantics being released as 0.5 (see #389), but it got quietly released as 0.4.8 before anything was resolved.

[rossabaker]

It's more fiddly than that: if we don't release when we return the connection to the idle state, then if the pool is full of idle connections, no connection can be taken for a key which has no idle connection. The crufty old http4s-blaze connection pool handles this by releasing an idle connection for another key to make room.

We need to decide whether to fix this in place in an 0.4.9 -- which would be the second consecutive semantic change on that release line -- or push toward 0.5.