Fix/pub sub

[buntec]

Attempts to fix #100 and #104.

[lenguyenthanh]

Thanks, I have the same issue. And this fixed the issue for me.

[armanbilge]

Sorry if I missed it. What replaces this .timeout(...) call? I don't think I saw any new .timeout(...) calls introduced above.

[buntec]

Oops, you are right, will try to fix it.

[lenguyenthanh]

I took a quick look, and have a naive solution, which is add timeout to request function of each Connection class. Would it work?

M core/shared/src/main/scala/io/chrisdavenport/rediculous/RedisConnection.scala
@@ -40,7 +40,7 @@ object RedisConnection{
       }
     } 
   }
-  private[rediculous] case class PooledConnection[F[_]: Concurrent](
+  private[rediculous] case class PooledConnection[F[_]: Temporal](
     pool: KeyPool[F, Unit, Socket[F]], timeout: Duration
   ) extends RedisConnection[F]{
     def runRequest(inputs: Chunk[NonEmptyList[ByteVector]], key: Option[ByteVector]): F[Chunk[Resp]] = {
@@ -52,6 +52,7 @@ object RedisConnection{
           case _ => Applicative[F].unit
         }
       }.rethrow
+      .timeout(timeout)
     }
   }
 

[buntec]

Yes, that's what I've done now. I've also sealed the RedisConnection trait to ensure exhaustive matches in RedisPubSub. Or is that going too far?

[lenguyenthanh]

I've also sealed the RedisConnection trait to ensure exhaustive matches in RedisPubSub. Or is that going too far?

I think that's a good idea.

[armanbilge]

Wouldn't a more straightforward fix be to keep TimeoutConnection, add it to the pattern match, and handle that case recursively?

I think adding sealed is good, but it's a source-breaking change that probably Chris should sign-off on.

If I understand correctly: for the record, this sort of bug would be prevented with proper linting enabled?

[buntec]

Agree, that seems better!

[lenguyenthanh]

If I understand correctly: for the record, this sort of bug would be prevented with proper linting enabled?

I would believe so.