race condition when returning and then leasing a connection

[stchang]

I believe I'm still seeing a race condition when trying to return and then quickly leasing a connection.

The following program:

#lang racket
(require "redis.rkt")

(define p (make-connection-pool))

(define conn (connection-pool-lease p))

(printf "got connection: ~a\n" (eq-hash-code conn))
(printf "owner: ~a\n" (eq-hash-code (redis-connection-single-owner conn)))
(printf "current thread: ~a\n" (eq-hash-code (current-thread)))

(connection-pool-return p conn)

(define conn2 (connection-pool-lease p))

(printf "got connection: ~a\n" (eq-hash-code conn2))
(printf "owner: ~a\n" (eq-hash-code (redis-connection-single-owner conn2)))
(printf "current thread: ~a\n" (eq-hash-code (current-thread)))

Produces the following output:

got connection: 20173
owner: 20172
current thread: 20172
got connection: 20173
owner: 592818
current thread: 20172

So the second time I get the connection, the owner is wrong? Seems like the pool gave out the connection while it was still cleaning it? Maybe we need another lock?

@m4burns, can you replicate this (or did I mess something up)?

[m4burns]

Yeah, actually, I can't believe I overlooked this: connection-pool-lease first checks if there's an entry in the key=>conn table for the current thread, and immediately returns the connection already leased by the current thread if so. However, after connection-pool-return is called, the connection remains in the table until release-conn has a chance to clean it up in the control thread. The removal isn't done in connection-pool-return since that would make the call to hash->list in the control thread have undefined behaviour.

I implemented it this way because I didn't want a single thread to be able to lease many distinct connections. There are two fixes:

• Check that the current thread is in the table and the mapped value has a redis-connection-single-owner field eq? to the current thread.
• Allow threads to lease multiple connections (this will require some modifications: the implicit release of connections at thread termination will need to handle a list of connections).

I think it makes sense for a single thread to only need one connection from the pool; I have been opening up long-lived connections for pub/sub separately from the connection pool.

[m4burns]

Thanks for finding that and sorry about the bugs! :/

[stchang]

Thanks for the reply.

I think it makes sense for a single thread to only need one connection from the pool; I have been opening up long-lived connections for pub/sub separately from the connection pool.

Don't these statements sort of contradict each other? What if I get a connection from the pool, subscribe to something on that connection. Now I want to do other stuff (in the same thread), so I'll need another connection so I ask the pool, but it just give me back the same connection.

It seems somewhat awkward to require the user to get connections from the pool, except in the case of pub/sub?

Maybe the pool should give out more than one connection per thread?

Thanks for finding that and sorry about the bugs! :/

No worries, it's an inevitable part of writing software. Thanks for all your help. I think your improvements are great!

[stchang]

Just checking, @m4burns are you working on a fix? I definitely don't mind taking a crack if you don't have time, but just didn't want to duplicate effort.

[m4burns]

I don't have time today. Go for it.

[stchang]

Alright I will, thanks.