[WIP] Add support for Ring websocket API

[weavejester]

This PR should not yet be merged, as the Ring websocket API is not stable. However, I wanted to implement the alpha websocket API for different adapters in order to head off any difficulties or flaws in the API design.

Implementing this for http-kit proved to be more difficult than I expected, as http-kit abstracts things like the close handler, and the Ring API is a little more low level. As an overview:

• I added a pongHandler to handle pong frames
• I added a closeRingHandler to handle close frames and pass the raw code and reason data
• I added functionality to send ping and pong frames explicitly via send
• I used the hato library for testing the Ring websockets in the tests, as the async.http.client one didn't support custom codes and reason strings in close frames.

I use some private middleware to detect Ring websocket responses and convert them into a http-kit channel.

This PR will eventually close #541.

[weavejester]

I think the failure in the main test is unrelated to my changes. It looks like the test-server-status test is expecting the server to be :stopped when it's currrently :stopping, so maybe a slowdown on the CI server caused the test to fail.

[ptaoussanis]

Hi James, thanks for this! Will try take a closer look tomorrow, but in the meantime wanted to respond to this-

I think the failure in the main test is unrelated to my changes. It looks like the test-server-status test is expecting the server to be :stopped when it's currrently :stopping, so maybe a slowdown on the CI server caused the test to fail.

Correct, the failure's very possibly unrelated. Looks like the timing for that test may not leave much margin for error if CI is slow, I'll bump the allowed time.

[ptaoussanis]

This PR should not yet be merged, as the Ring websocket API is not stable. However, I wanted to implement the alpha websocket API for different adapters in order to head off any difficulties or flaws in the API design.

👍 Thanks for the clear status, and all your work on this James!

Took a quick skim at the current PR, looks broadly reasonable from my end.

Only concern that stuck out - it looks like your wrap-ring-handler fn needs access to ring.websocket/Socket, which'll mean a dependency on ring/ring-core?

Is access the Socket protocol the only reason for the dependency? If so, would there be any way to avoid that?

One of http-kit's major value propositions is it's lack of dependencies.

Edit: forgot to add, I've bumped the failing test's allotted time - so hope that issue might resolve if you rebase your PR.

[borkdude]

I wouldn't be too happy about the hard-coded dependency on ring since this has an impact on graalvm binaries like babashka, their size would become bigger (and there is more code to worry about getting right in relation to graalvm native-image). If possible I'd appreciate if this could be done simply by passing a function to run-server which would provide the necessary functionality, for example.

[weavejester]

This is exactly the sort of feedback I was hoping to get, as it's something I wouldn't have considered otherwise. So first of all, thank very much for taking the time to review this.

What if I split the relevant protocols out into their own library, so I'd have a org.ring-clojure/ring-websocket-protocols library, that would contain only the following namespace and nothing else:

(ns ring.websocket.protocols)

(defprotocol Listener
  (on-open [listener socket])
  (on-message [listener socket message])
  (on-pong [listener socket data])
  (on-error [listener socket throwable])
  (on-close [listener socket code reason]))

(defprotocol PingListener
  (on-ping [listener socket data]))

(defprotocol Socket
  (-open? [socket])
  (-send [socket message])
  (-ping [socket data])
  (-pong [socket data])
  (-close [socket code reason]))

(defprotocol AsyncSocket
  (-send-async [socket message succeed fail]))

While this would still add a dependency, it would be as minimal as possible with no code beside the relevant protocols. Would this be acceptable?

[borkdude]

Seems like an idea. I wonder if there's ways to do it without adding dependencies at all. "Just use maps"? Or maybe that would be too brittle?

[borkdude]

I guess another idea would be to add:

1 a protocol to http-kit
2 a separate (optional) dependency which bridges http-kit and ring-core-websockets

Maybe that's too much overkill though

[borkdude]

A note: ring-core pulls in more transitive dependencies as well:

ring/ring-core 1.11.0-alpha4
  . ring/ring-codec 1.2.0
  . commons-io/commons-io 2.13.0
  . org.apache.commons/commons-fileupload2-core 2.0.0-M1
    . commons-io/commons-io 2.13.0
  . crypto-random/crypto-random 1.2.1
    . commons-codec/commons-codec 1.15
  . crypto-equality/crypto-equality 1.0.1

Maybe another good argument to split things up.

My question would be: would the existing websocket functionality (as of before this PR) keep on working, when you do not include the full ring-core dependency? (If not, that would be breaking, e.g. current babashka users)

[weavejester]

Seems like an idea. I wonder if there's ways to do it without adding dependencies at all. "Just use maps"? Or maybe that would be too brittle?

We could do something like this, where both the listener and socket are maps of functions:

{:ring.websocket/listener
 {:on-open (fn [{:keys [send ping open?]}]
             (when (open?)
               (ping) 
               (send "Hello World"))}}

The problem is that this would be less performant than using protocols, the functions wouldn't be enforced by the compiler, and it feels less idiomatic overall. I think my preference would still lean toward having a minimal protocol library.

My question would be: would the existing websocket functionality (as of before this PR) keep on working, when you do not include the full ring-core dependency? (If not, that would be breaking, e.g. current babashka users)

Currently no, because org.httpkit.server requires ring.websocket. However, we only need ring-core for the Ring websocket protocols, so if we pull them out, no ring-core dependency would be needed.

[borkdude]

Just to be clear, websocket functionality will keep on working as it is right now (on master), without adding a dependency on ring-core, but just a small dependency on a protocol-only dependency? Sounds good enough to me!

[borkdude]

With "now" I mean as things are on master, not as they are in this PR.

[weavejester]

Just to be clear, websocket functionality will keep on working as it is right now (on master), without adding a dependency on ring-core, but just a small dependency on a protocol-only dependency? Sounds good enough to me!

Yes, exactly.

[ptaoussanis]

While this would still add a dependency, it would be as minimal as possible with no code beside the relevant protocols. Would this be acceptable?

That'd definitely be preferable, but I do think we should try rule out dependency-free alternatives first.

@weavejester James, http-kit already contains a fair bit of conditional code (e.g. here).

Perhaps instead of unconditionally pulling in the relevant Ring dependency, we could instead check if it's already loaded/present and act accordingly?

[weavejester]

If we want a purely dynamic load, then for the socket, we could use a metadata protocol:

(defn- channel->ring-socket [^AsyncChannel ch]
  (with-meta
   {:channel ch}
   {'ring.websocket.protocols/open? (fn [_] (not (.isClosed ch)))
    ...}))

I haven't tried it before, but I believe this should work. And for the listener, we could use a var lookup:

(defn- channel->listener [^AsyncChannel ch listener]
  (require 'ring.websocket.protocols)
  (let [on-message (var-get (find-var 'ring.websocket.protocols/on-message))]
    (.setReceiveHandler ch #(on-message listener ch (->ring-message %)))
    ...))

There would probably be some performance loss, but I'm unsure if that would matter much for something like websockets that are mostly going to be limited by I/O.

However, regardless of whether we use that, I think it's still a good idea to separate out the Ring websocket protocols into their own library.

[borkdude]

Better to avoid "inline" require in functions, it does not work well with GraalVM native-image (run-time requires don't work + run-time requires yield insanely big images because GraalVM cannot analysis that stuff isn't used). Probably better to use utils/compile-if to do this work at compile time so require isn't called anymore at runtime.

[weavejester]

So something like:

(defn- wrap-ring-websocket [handler]
  (utils/compile-if (require 'ring.websocket.protocols)
    ;; ring websocket code
    handler))

Would that work in GraalVM? I guess we could also use reify instead of metadata protocols in that case for the socket.

[borkdude]

Yeah, that would work, I think.

wrap-ring-websocket returns nil when ring-core is not on the classpath in this case. Would the existing (as now on master) websocket functionality be affected by that?

[weavejester]

wrap-ring-websocket returns nil when ring-core is not on the classpath in this case. Would the existing (as now on master) websocket functionality be affected by that?

Ah, sorry, my example probably wasn't clear. The ;; ring websocket code comment was a placeholder for all the ring websocket code. If there is no ring websocket protocols included, it defaults to returning handler (i.e. doing nothing).

[borkdude]

Gotcha!

[borkdude]

Small details, the condition in compile-if must return a truthy value and require normally returns nil so something like:

(do (require 'ring.websocket.protocols) true)

would be necessary as the condition.

[weavejester]

I've updated the code so that Ring WebSockets are an optional dependency, and that dependency is a small library consisting of only protocols. I think the tests need a little more work to get them passing more reliably, and the Ring dependencies will need updating once we hit an RC, but overall I think it's done for now.

[borkdude]

I'd be interested in one test suite which checks if the existing websocket stuff still runs without the ring dependency as well.

[weavejester]

That's a good point. Maybe we just add a Leiningen profile that omits the dependency, and add that to the GitHub Actions tests.

[ptaoussanis]

@weavejester Hi James, happy new year! Just confirming that this is ready to go from your side? I'm happy to update the deps and add the remaining deps-free tests when merging.

[weavejester]

Yes it should be ready aside from the updated deps and the deps-free tests that you mentioned.

I also separated out the websocket protocols into a minimal org.ring-clojure/ring-websocket-protocols dependency, so if people want to use websockets with http-kit, but wish to avoid ring-core, that's an option for them.

[ptaoussanis]

Merged manually 👍 On master now, will be included in the next release (v2.8.0-RC1, planned for later this month).

Thanks again for all the work on this! 🙏