Fork me on GitHub
#code-reviews
<
2021-12-06
>
Noah Bogart17:12:27

I’ve been working on rewriting the sente websocket handlers in my app and want some feedback on how I’m dealing with side effects. As written by the previous maintainer, there were multiple global state atoms and lots of mutation, so in my rewrite, i’ve gotten it down to a single state atom and I’ve tried to move as much logic as possible into functions that can be called in one swap!. Here’s a relatively simple example from the codebase:

(defn handle-send-message [lobbies gameid message]
  (if-let [lobby (get lobbies gameid)]
    (-> lobby
        (send-message message)
        (->> (assoc lobbies gameid)))
    lobbies))

(defmethod ws/-msg-handler :lobby/say
  [{{user :user} :ring-req
    uid :uid
    {:keys [gameid text]} :?data}]
  (let [lobby (app-state/get-lobby gameid)]
    (when (and lobby (in-lobby? uid lobby))
      (let [message (core/make-message {:user user :text text})
            new-app-state (swap! app-state/app-state
                                 update :lobbies
                                 #(-> %
                                      (handle-send-message gameid message)
                                      (handle-set-last-update gameid uid)))
            lobby? (get-in new-app-state [:lobbies gameid])]
        (send-lobby-state lobby?)))))

Ben Sless18:12:02

Biggest issue I see here is you're touching the game state more than once, and it might change while you're looking at it

Noah Bogart18:12:22

you mean with get-lobby? Yeah, I’m not a big fan but it seemed best to kick out early instead of doing anything extra in any case where the lobby doesn’t exist or the user isn’t in the lobby.

Noah Bogart18:12:32

or do you mean something else?

Ben Sless18:12:45

that's exactly what I meant

👍 1
Ben Sless18:12:09

Since this is kinda event driven you can model it with channels. A bit different from your current model, but bear with

Ben Sless18:12:58

This models your system "purely"

(defn state-loop
  [step effect init input output]
  (go-loop [state init]
    (let [v (<! input)]
      (if (nil? v)
        (close! output)
        (let [state' (step state v)
              out (effect state v)]
          (when out (!> output out))
          (recur state'))))))

Ben Sless18:12:51

Then everyone writes to the input channel and you have a single writer, state is always consistent

Ben Sless18:12:14

effects are completely reified and written to the output channel, handled by an effect handler in another loop

Noah Bogart18:12:52

And step in this case is my update function?

Ben Sless18:12:23

Then your update function is pure and you can unit-test it as much as you like. your effect function is pure, you can test it as much as you like

Ben Sless18:12:04

added benefit - your effect handler can be dead simple

Noah Bogart18:12:35

Feels a lot like reframe! I’ve felt the desire for a reframe-like system in this code base a couple times, hah. Thanks for the input, I’ll give this a go and see how it feels.

Ben Sless18:12:43

Sure. Feel free to yell at me later if I sent you down a path of tribulations

Ben Sless18:12:30

It's a nice model, though. You can very easily put little state machines in your state per entity , so if you reify your state transitions you end up with an extremely clear implementation

Ben Sless18:12:56

Each state transition has a possible emitted effect associated with it based on the input

Ben Sless18:12:31

This is pretty much what servers do

Noah Bogart19:12:22

okay, back from lunch. looking at your state-loop more closely, you’re passing in the state map, the two functions, and then two channels. the go-loop returns a channel and will only exit if the input channel spits out nil. how many times am i calling this function? do call this in every ws handler?

Noah Bogart19:12:37

i’m certainly at the edge/limit of my core.async knowledge, lol

Noah Bogart19:12:56

(i see that it’s 9:36 pm where you are, so please don’t rush to answer me.)

Ben Sless19:12:12

State loop is the event loop. You call it once when the application starts

👍 1
Ben Sless19:12:31

You shut it down by closing the input channel

Noah Bogart19:12:26

in my example, when I call swap! i’m passing a function that will update the state at that moment. what are step and effect when calling state-loop, then?

Ben Sless19:12:57

You can think about it like two mathematical series State[n] = step(State[n-1], Event[n-1]) Effect[n] = effect(State[n], Event[n]) They are infinite. Your entire application state is encapsulated in that life cycle loop. step would be the function you pass to swap! minus the input.

(fn step [current-state current-input]) -> new-state
effectwould be send-lobby-state which only emits data and an effect handler would actually send it to whoever

Noah Bogart19:12:56

oh! okay, yeah, that’s very interesting

Ben Sless19:12:05

Something like:

(fn step [state event]
  (let [{{user :user} :ring-req
         uid :uid
         {:keys [gameid text]} :?data} event]
    (let [lobby (app-state/get-lobby state gameid)]
      (cond-> state
        (and lobby (in-lobby? uid lobby))
        (update :lobbies #(-> lobbies
                              (handle-send-message gameid (core/make-message {:user user :text text}))
                              (handle-set-last-update gameid uid)))))))

Ben Sless20:12:27

And I think you'll need to modify the loop in your case for effect to take the new state as an argument and not the old one

Ben Sless20:12:11

I also prefer this slight refactor:

(defn -lobbies [state event]
  (let [{{user :user} :ring-req
         uid :uid
         {:keys [gameid text]} :?data} event]
    (when-let [lobby (app-state/get-lobby state gameid)]
      (when (in-lobby? uid lobby)
        (->
         state
         :lobbies
         (handle-send-message gameid (core/make-message {:user user :text text}))
         (handle-set-last-update gameid uid))))))

(fn step [state event]
  (let [lobbies (-lobbies state event)]
    (cond-> state
      lobbies (assoc :lobbies lobbies))))

Noah Bogart20:12:15

so this is one case and as you’ve written it i think it would work. but i have 20+ web socket handlers that each do something subtly different. in this case step would have to be closer to update, right? something like (defn step [state [k args]] (update state k args)) and then when i need to actually use this, i’d write (>! input [:lobbies #(let [{{user :user} :ring-req %] …)] or something similar.

Ben Sless20:12:16

Or you could implement a multimethod which handles each event differently instead of closing over it like you did here

Ben Sless20:12:43

(defmulti handle-event (fn [_state event] (:type event)))

(fn step [state event]
  (let [result (handle-event state event)
        k (:type event)]
    (cond-> state
      result (assoc k result))))

Ben Sless20:12:00

or something like that

Noah Bogart20:12:16

hmmmm interesting

Ben Sless20:12:01

I don't think this is a terrible model, you just have to keep to a convention where handle-event returns a result or nil. If it returns a result, you update the state for the event, if nil, state is unchanged

👍 1
Noah Bogart20:12:14

haha i just realized we’re absolutely recreating re-frame right now

Ben Sless20:12:24

Never used it

Noah Bogart20:12:25

they have a whole system for subscribing to changes which we’ve not discussed, but their model is creating multimethod-like functions tied to specific keywords, and then when receiving an event, the associated function for that keyword is called and performs a “pure function” change on the state, and only after all event handlers are finished is the change persisted to the in-memory db

Noah Bogart20:12:21

you’ve proposed a “change state” multimethod and i think i’d need a “send updates to clients” multimethod that uses the changed state, or at least some system for deciding per-event which information to send. (don’t send message updates to users not in the lobby, etc) and then any ws message would be converted to an input for the state-loop channel

Noah Bogart20:12:31

very interesting and compelling

Noah Bogart20:12:58

would certainly clean up a lot of this messy “is it pure or not?” song-and-dance

🎉 1
Ben Sless20:12:15

Oh you absolutely will need another multimethod. I'd say you need two, one pure to reify the effects, one impure to actually do side effects with no awareness of state

👍 1
pithyless20:12:11

Fun fact: in 2015 re-frame replaced their core.async event-loop with a custom queue (primarily for performance reasons and to have better control when the browser can re-render): https://github.com/day8/re-frame/commit/420e42aacccbac2d81fedc5ff861442a4ce70c1d

pithyless20:12:03

You may also find that sometimes event need "extra" input from side-effecting code (e.g. current timestamp, etc). This would be a co-effect in re-frame parlance.

👍 1
Ben Sless20:12:15

I didn't want to bring that up but yes, in high throughput or low latency you might have to tear out core.async, but it'll enforce a good model and level of abstraction

👍 1
Ben Sless20:12:39

Aren't co-effects just events you feed back to yourself?

Noah Bogart20:12:10

we average 100 unique users a day, so i don’t think i need to worry about that at the moment lol, but that’s very good to know for the future

Ben Sless20:12:53

I hope you won't have latency issues, but the model is too good to pass over imo

Ben Sless20:12:24

Will this code run server-side?

Noah Bogart20:12:51

yes, this is the server code

Ben Sless20:12:36

a. The JVM is a tough beast and can give you great performance b. Use JDK17 if you can

👍 1
pithyless20:12:35

> Aren't co-effects just events you feed back to yourself? No, from my understanding (warning: I don't use re-frame), they are a way to parameterize and represent where your inputs come from the outside world. (In contrast to effects, that represent what should happen to the outside world as a result)

Ben Sless20:12:27

Do they need awareness of current state? If you can run them before you enter the main loop then you're still in pure world

Noah Bogart20:12:34

no. they’re functions that perform some impure action that you then refer to in your effect handler, which will automatically perform the impure action before entering your handler, giving you access to the impure data without having to pollute the effect handler directly (https://day8.github.io/re-frame/Coeffects/)

Noah Bogart20:12:11

(specifically the “inject-cofx” and “meet reg-cofx” sections)

Noah Bogart18:12:25

there are a couple functions not included here because they seemed obvious enough for their name. to describe in words my intentions: when someone sends a message in a lobby, i make sure the user is in the lobby, convert the given text into a “message” map, and then within the swap! call, I’m updating the lobby only if the lobby exists at the time of the swap! and otherwise just returning the existing lobbies (to avoid a race condition where someone sends a message at the same time as the lobby is closed/deleted and thus there’s no lobby to update). i check to see if the lobby does exist after performing the update (`lobby?`), and then send-lobby-state only sends an update to the clients if the argument isn’t nil