Questions and code improvement suggestions

[edubrovski]

I have some questions and suggestions for improving the code. Feel free to ignore stylistic suggestions because they're subjective.

1. I don't think we should expose IntHelper as a part of public API, I'd mark it as private[resourcepool].

2. Do we really need implicits in our public API (IntHelper, ResourcePoolOps)? IMO introducing an implicit is justified only if it's going to be used a lot of times. How many times a user is going to construct a pool in his code? Probably once or twice. So I think it's much easier for the user to have an explicit method and not think about what they need to import.

3. If I understand correctly, cancellation works differently for ResourcePoolOps.resource than for ResourcePool.get. cats.effect.Resource.apply makes acquire and release uncancelable. I think this change in cancellation semantics can be a surprise for the users, so I'd mention it in the scaladoc for ResourcePoolOps.resource.

[edubrovski]

4. Something I don't understand is why do we count in tailRecM, for example

   0.tailRecM { count =>
   ...
   ...
   (count + 1).asLeft[Unit].pure[F]

   Is this for debugging? I would just do

   
   ().tailRecM { _ =>
   ().asLeft[Unit].pure[F]
   }

[t3hnar]

could be useful for debugging, and in short it does not provide more overhead comparing to what you propose :)

also it's nice when type differs from a happy path

[edubrovski]

What happens if we fail to allocate the resource and there are tasks (Deferreds) in the queue?

if (entries.isEmpty) {
  state.stage match {
    case stage: State.Allocated.Stage.Free =>
      apply(stage) { ().pure[F] }
    case stage: State.Allocated.Stage.Busy =>
      apply {
        State.Allocated.Stage.free(List.empty)
      } {
        stage
          .tasks
          .foldMapM { task =>
            task
              .complete(a.asLeft)
              .void
          }
      }
  }
} else {
  apply(stage) { ().pure[F] }
}

Looks like we complete all Deferreds with an error. What if there's just a temporary glitch? For example, when a KafkaConsumer gets created it tries to establish a connection to the broker. If there's a temporary network problem and one allocation fails because of it, all client fibers waiting for a consumer will get an error.

Shouldn't we retry allocation to avoid such situation?

[t3hnar]

all client fibers waiting for a consumer will get an error.

if there are no other consumers in the pool - all get an error, but attempts after - will initiate allocation of a new consumers.

If maxSize > 1, only one attempt will fail.

you can implement retry on the level of passed resource, we don't need to encode this into the pool.

if (entries.isEmpty) {

means maxSize=1

[edubrovski]

Thanks for explanations! I couldn't find any bugs in the code. I've reviewed roughly half of it (I didn't review parts of code related to graceful shutdown).