Atomically incrementing a value in a map in Clojure

Question

I am working out a bit of Clojure code that will take a ref to a map and increment a key value pair in the map. I think I am using ref correctly, but Im not sure about atom. Do I need to use swap! to be more idiomatic? I am new to STM and Clojure, does this look thread-safe / sane? What am I missing?

(defn increment-key [ref key]
    (dosync
        (if (= (get @ref key) nil)
            (alter ref assoc key (atom 1))
            (alter ref assoc key (atom (inc @(get @ref key)))))))

(defn -main [& args]
    (def my-map (ref {}))
    (increment-key my-map "yellow")
    (println my-map)
    (increment-key my-map "yellow")
    (println my-map))

Prints

$ lein run
#<Ref@494eaec9: {yellow #<Atom@191410e5: 1>}>
#<Ref@494eaec9: {yellow #<Atom@7461373f: 2>}>

Answer 1

Don't always create new atoms

My first comment is that your use of atom seems quite wrong to me: atoms are mutable entities which you're supposed to mutate via e.g. swap! or reset!, which alter the value the atom points to without changing the atom reference itself. What your code is doing is to create new atoms at every call, i.e. you change the atom reference completely. A more proper approach would be:

(defn increment-key [ref key] 
  (dosync 
    (if-let [current (@ref key)]        ; maps are functions of their keys, no need for 'get'
      (do (swap! current inc) @ref)     ; 'do' here is only used to return the current map
      (alter ref assoc key (atom 1))))) ; change the atom in place

Do you need an atom?

My second comment is that you most likely don't need atom at all, unless you want to also use it directly as shared state among different threads. If you only plan to use the ref map as the shared state, you can just drop the atom and make your code easier to handle. One of the benefits of Clojure immutable data structures is that values are inherently thread safe. The following code would be perfectly thread safe:

(defn increment-key [ref key]
    (dosync
        (if-let [current (@ref key)]
            (alter ref assoc key (inc current))
            (alter ref assoc key 1))))

Do you need a ref?

A last comment is about choosing the right tool for the job: ref is intended for coordinated state change. This means that it helps you in case you have several mutable state references that need to mutate all in a single transaction, which you create with dosync. Your code doesn't seem to need transactions at all, it just needs a single mutable state reference to change atomically, i.e. in a thread safe manner. This is better achieved with a straight atom, which relieves you from the need to establish a transaction:

(defn increment-key [atm key]
  (if-let [current (@atm key)]
    (swap! atm update key inc)
    (swap! atm assoc key 1)))

A next step might be to let the change to be asynchronous by using an agent instead of an atom, but that's slightly beyond the scope here IMO.