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.

https://github.com/typelevel/keypool/blob/41c4f5177f8b1e904c19d48b95cfd26fd9d9bbea/core/src/main/scala/org/typelevel/keypool/KeyPool.scala#L298-L312

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.