Request for code review: Safely starting and stopping a server in the REPL

Hello, everyone!

In every project where I work with a server, I want to be able to start and stop the server from the REPL without too much fuzz.

Objectives:

  • Start and stop server from the REPL
  • Avoid getting into weird states where the server is still running, but I don’t have a reference to it.

Specific questions:

  • Did I manage to write the start and stop functions thread safe?
  • Comments on using a promise inside the swap function?

Code:

(ns eu.teod.hello-test
  (:gen-class)
  (:require [ring.adapter.jetty]))

(defn handler [req]
  {:status 200
   :headers {"ContentType" "text/html"}
   :body "Hello World"})

(defonce server (atom nil))

(defn start-server! [{:keys [port]}]
  (let [retval (promise)]
    (swap! server
           (fn [old-server]
             (if old-server
               (do (deliver retval ::server-already-running)
                   old-server)
               (do (deliver retval ::server-started)
                 (ring.adapter.jetty/run-jetty #'handler
                                             {:port port
                                              :join? false})))))
    @retval))

(defn stop-server! []
  (let [retval (promise)]
    (swap! server
           (fn [old-server]
             (if-not old-server
               (do (deliver retval ::server-already-stopped)
                   old-server)
               (do (deliver retval ::server-stopped)
                   (.stop old-server)
                   nil))))
    @retval))

(comment
  (start-server! {:port 3000})
  (stop-server!)
  )

(defn -main
  "I don't do a whole lot ... yet."
  [& args]
  (start-server! {:port 80}))

Feedback welcome :slight_smile:

Teodor

Hum, that’s an interesting little problem. I’m not totally sure, but I don’t think your code is thread safe.

As swap! says:

Note that f may be called
multiple times, and thus should be free of side effects. Returns
the value that was swapped in.

In your case, since you are using a promise and deliver inside the swap!, that is effectively side effect. Also in your case, run-jetty is side effect, because it binds to the IO port, which is a global thing.

So what could happen, I believe, is that you have two threads check if old-server and they both see that it is nil, thus they both deliver ::server-already-running, and then they both call run-jetty on the same port, one of them would probably fail, I assume run-jetty throws if called on the same port? Or they’d both succeeds if they were using different ports and now you’d have two servers. They would also both deliver to the promise.

In either case, when the f is done, swap! will ensure that only one of them wins the at writing the value to the atom, the one which fail will retry f, where you’d lose the reference to the server it created if that had succeeded, now in your case it would move to execute the if old-server is true branch, and it will try to deliver ::server-already-running, but you can’t deliver to a promise more then once, so even though this is what it ended doing, your function will return the first thing it delivered, and you will see that both threads return ::server-started.

Now this behavior might not cause you issues per say, but it isn’t thread safe. At least from my understanding of swap!

In your case, might be better to use locking.

1 Like

Are you sure that you need all this? I’d suspect the call to .stop actually blocks until the server is fully stopped? At least that would be common behavior for JVM stuff?

1 Like

Good morning, everyone!

Thanks for your replies. I see that we’ve found two different issues.

  1. swap might be called several times, and is persumed to be side-effect free. Thus it’s not the right choice for side-effecting functions – which is what I persumed I needed. (@didibus)
  2. I might not need to be careful about this at all, because it might already be handled on the JVM side of the table (@thheller).

I think I’ll have something to learn experimenting with both. So I’ll start out writing a version with locking, and let’s see how that goes.

From my limited clojure knowledge, aren’t agents the threadsafe counterpart to atoms?

@didibus, I believe I’ve validated your concern.

;; What happens if I try to start 10 servers at the same time, at the same port?

;; The outcome I'd really want is just one started and 9
;; returning ::server-already-running

;; I suspect I might be able to trigger some errors due to side effects under
;; `swap`. Let's go!

(comment
  (do
    (stop-server!)
    (let [latch (promise)]
      (dotimes [i 10]
        (future
          (deref latch)
          (start-server! {:port 3000
                          :msg [:server i]})))
      (deliver latch ::start-test)))
  )

;; Prints
;;    2020-01-18 10:10:08.084:INFO:oejs.Server:clojure-agent-send-off-pool-10: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-8: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-12: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-7: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-11: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-6: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-4: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-3: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-5: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.085:INFO:oejs.Server:clojure-agent-send-off-pool-9: jetty-9.4.22.v20191022; built: 2019-10-22T13:37:13.455Z; git: b1e6b55512e008f7fbdf1cbea4ff8a6446d1073b; jvm 1.8.0_232-8u232-b09-0ubuntu1~18.04.1-b09
;;    2020-01-18 10:10:08.154:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-5: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.154:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-9: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.157:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-3: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.160:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-6: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.160:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-12: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.160:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-8: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.162:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-7: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.162:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-4: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.162:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-11: Started [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}
;;    2020-01-18 10:10:08.163:INFO:oejs.Server:clojure-agent-send-off-pool-11: Started @61513ms
;;    2020-01-18 10:10:08.163:INFO:oejs.AbstractConnector:clojure-agent-send-off-pool-10: Stopped [email protected]{HTTP/1.1,[http/1.1]}{0.0.0.0:3000}

Hmm, good point. Perhaps there’s a “swap-like” function that blocks when different threads try to access it, instead of running and later re-running?

@didibus,

I built a solution using locking. From my testing, it seems to work as it should. Thanks for your insight!

(ns eu.teod.scratch.concurrent-2
  (:require [ring.adapter.jetty]))

(defn handler [req]
  {:status 200
   :headers {"ContentType" "text/html"}
   :body "Hello World"})

(defonce server (atom nil))


;; I wrap ring.adapter.jetty/run-jetty to collect all the things it starts, so
;; that I can shut them all down without restarting the REPL. Feel free to skip
;; to the real stuff.

(defonce all-started-servers (atom []))

(do
  (defn run-jetty [& args]
    (let [server (apply ring.adapter.jetty/run-jetty args)]
      (swap! all-started-servers conj server)
      server))
  (alter-meta! #'run-jetty
               (fn [current-meta]
                 (merge (meta #'ring.adapter.jetty/run-jetty)
                        (select-keys current-meta [:line :column :file :name :ns]))))
  #'run-jetty
  )

;; real stuff

(defn start-server! [{:keys [port msg]}]
  (locking server
    (if @server
      ::server-already-running
      (let [new-server (run-jetty #'handler
                                  {:port port
                                   :join? false})]
        (reset! server new-server)
        ::server-started))))

(defn stop-server! []
  (locking server
    (if @server
      (do (.stop @server)
          (reset! server nil)
          ::server-stopped)
      ::server-already-stopped)))

(comment
  (defonce results (atom []))
  (do
    (reset! results [])
    (stop-server!)
    (let [latch (promise)]
      (dotimes [i 10]
        (future
          (let [ret (start-server! {:port 3000
                                    :msg [:server i]})]
            (swap! results conj ret))))
      (deliver latch ::start-test)))
  @results
  (frequencies @results)
  ;; => #:eu.teod.scratch.concurrent-2{:server-started 1, :server-already-running 9}
  )

Thanks, everyone for the replies. I find concurrency to be a quite hard topic, but exploring concurrency with the REPL can be really rewarding.

Have a nice day!

It depends what you want to do. Both atom and agent are concurrency construct and allow you to build thread safe code, but for different scenarios.

Atoms are meant to allow two or more threads to update the value in the atom in a thread safe and lock free way, using optimistic locking. This is synchronous, and thus strongly consistent. After the call to swap! returns you can immediately read the atom and see the result.

Agents are meant to allow one or more threads to update the value in the agent in a thread safe way, but it isn’t lock free, and uses pessimistic locking. The major difference with atom being that it does the update asynchronously, and is thus eventually consistent. After the call to send, reading the value of the agent might still return you the old value.

That said, you can do side-effects in the send/off fns of agents, because they run one after the other over the agent. The next one waits for the previous one.

In this case, an agent could be used as well, because it will effectively synchronize the sends/offs. The difference with locking is two fold:

  1. Locking is more performant, not only it has less overheard, but agents don’t just guarantee single actor over the agent, they also guarantee dispatch order, the next sent action is guaranteed to be the next one to run. Locking will pick any next one.
  2. Maybe the more important distinction in this case is that agents are asynchronous and eventually consistent. But here, we want start and stop to be synchronous functions, and we want their result to always be up to date. You can do it with agent using await, but in my opinion, agent is overkill and less appropriate.

Thanks for the clarification. I had thought that atoms weren’t threadsafe, so I learned something today :slight_smile: