Redis store does not appear to be thread-safe

[iamFIREcracker]

Steps to reproduce:

• Load the necessary dependencies:

(ql:quickload '(:clack :lack :lack-session-store-redis :cl-redis))

• Setup a dummy "Hello, World" application:

(defparameter *app*
  (lambda (env)
    (declare (ignore env))
    '(200 (:content-type "text/plain") ("Hello, World"))))

• Setup the middleware chain, one that uses Redis as session storage:

(setf *app*
      (lack:builder
        (:session
         :store (lack.session.store.redis:make-redis-store :connection (make-instance 'redis:redis-connection
                                                                                      :host #(0 0 0 0)
                                                                                      :port 6379
                                                                                      :auth "auth")))
        *app*))

• Start the app:

> (defvar *handler* (clack:clackup *app*))
Hunchentoot server is started.
Listening on 127.0.0.1:5000.

• Run wrk against it:

$ wrk -c 10 -t 4 -d 10 http://127.0.0.1:5000
Running 10s test @ http://127.0.0.1:5000
  4 threads and 10 connections

Expected behavior:

• The test runs successfully, and the number of requests per second is reported

Actual behavior:

• Lots of errors are signaled; from unexpected errors:

Thread: 7; Level: 1

Redis error: NIL

ERR Protocool error: invalid multibulk length
   [Condition of type REDIS:REDIS-ERROR-REPLY]

Restarts:
  R 0. ABORT - abort thread (#<THREAD "hunchentoot-worker-127.0.0.1:53815" RUNNING {100154F693}>)

Frames:
  F 0.  ((:METHOD REDIS:EXPECT ((EQL :STATUS))) #<unused argument>) [fast-method]
  F 1.  (REDIS::SET "session:fdf15f46896842bcd84ef2e87d2321e97419f70e" "KDpQQ09ERSAxICg6SEFTSC1UQUJMRSAxIDcgMS41IDEuMCBFUVVBTCBOSUwgTklMKSk=")
  F 2.  ((:METHOD LACK.MIDDLEWARE.SESSION.STORE:STORE-SESSION (LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE T T)) #S(LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE :HOST #(0 0 0 0) :PORT 6379 :NAMESPACE "..
  F 3.  (LACK.MIDDLEWARE.SESSION::FINALIZE #S(LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE :HOST #(0 0 0 0) :PORT 6379 :NAMESPACE "session" :EXPIRES NIL :SERIALIZER #<FUNCTION #1=(LAMBDA (LACK.MIDDLEWARE.S..
  F 4.  ((LAMBDA (LACK.MIDDLEWARE.BACKTRACE::ENV) :IN "/Users/matteolandi/Workspace/lack/src/middleware/backtrace.lisp") (:REQUEST-METHOD :GET :SCRIPT-NAME "" :PATH-INFO "/" ...))

... to unexpected end of file:

Thread: 8; Level: 1

Redis error: end of file on #<FLEXI-STREAMS:FLEXI-IO-STREAM {100AF94323}>
   [Condition of type REDIS:REDIS-CONNECTION-ERROR]

Restarts:
  R 0. RECONNECT - Try to reconnect and repeat action.
  R 1. ABORT     - abort thread (#<THREAD "hunchentoot-worker-127.0.0.1:53818" RUNNING {1008949253}>)

Frames:
  F 0.  (REDIS::SET "session:5b391bd4699e86f5b6e757451572c0a3ba825557" "KDpQQ09ERSAxICg6SEFTSC1UQUJMRSAxIDcgMS41IDEuMCBFUVVBTCBOSUwgTklMKSk=")
  F 1.  ((:METHOD LACK.MIDDLEWARE.SESSION.STORE:STORE-SESSION (LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE T T)) #S(LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE :HOST #(0 0 0 0) :PORT 6379 :NAMESPACE "..
  F 2.  (LACK.MIDDLEWARE.SESSION::FINALIZE #S(LACK.MIDDLEWARE.SESSION.STORE.REDIS:REDIS-STORE :HOST #(0 0 0 0) :PORT 6379 :NAMESPACE "session" :EXPIRES NIL :SERIALIZER #<FUNCTION #1=(LAMBDA (LACK.MIDDLEWARE.S..
  F 3.  ((LAMBDA (LACK.MIDDLEWARE.BACKTRACE::ENV) :IN "/Users/matteolandi/Workspace/lack/src/middleware/backtrace.lisp") (:REQUEST-METHOD :GET :SCRIPT-NAME "" :PATH-INFO "/" ...))
  F 4.  ((:METHOD HUNCHENTOOT:ACCEPTOR-DISPATCH-REQUEST (CLACK.HANDLER.HUNCHENTOOT::CLACK-ACCEPTOR T)) #<CLACK.HANDLER.HUNCHENTOOT::CLACK-ACCEPTOR (host 127.0.0.1, port 5000)> #<HUNCHENTOOT:REQUEST {10071E47B..
  F 5.  ((:METHOD HUNCHENTOOT:HANDLE-REQUEST (HUNCHENTOOT:ACCEPTOR HUNCHENTOOT:REQUEST)) #<CLACK.HANDLER.HUNCHENTOOT::CLACK-ACCEPTOR (host 127.0.0.1, port 5000)> #<HUNCHENTOOT:REQUEST {10071E47B3}>) [fast-met..

A similar error was mentioned on the cl-redis GitHub's space (cl-redis/issues#19), to which the author replied that one the reasons why this might happen is when trying to share the same connection between multiple threads. So a took a look the implementation of the Redis store, and it looks like it is indeed sharing the same Redis connection between different HTTP requests (i.e. multiple threads).

Even though the following seems to be fixing the problem (for my scenario at least) I just wanted to see what your view on this was, before creating a pull-request:

diff --git a/src/middleware/session/store/redis.lisp b/src/middleware/session/store/redis.lisp
index 9e489d9..3ccfaec 100644
--- a/src/middleware/session/store/redis.lisp
+++ b/src/middleware/session/store/redis.lisp
@@ -54,9 +54,8 @@
 (defun redis-connection (store)
   (check-type store redis-store)
   (with-slots (host port auth connection) store
-    (unless (redis::connection-open-p connection)
-      (setf connection
-            (open-connection :host host :port port :auth auth)))
+    (setf connection
+          (open-connection :host host :port port :auth auth))
     connection))

 (defmacro with-connection (store &body body)

Few additional infos:

• OS

$ uname -vr
18.7.0 Darwin Kernel Version 18.7.0: Tue Jan 12 22:04:47 PST 2021; root:xnu-4903.278.56~1/RELEASE_X86_64

• SBCL

$ sbcl --version
SBCL 2.1.4