Ring websocket API compatibility

[weavejester]

I've been working on a Ring websocket API that's currently in experimental alpha. Would you be interested in a PR to support this API in rj9a, to be merged when the API is stable? This Ring API would be supported in conjunction with rj9a's existing implementation.

The reason I'm asking this while the Ring API is still experimental is that I'd like to try implementing the API for different third-party adapters, so I can get forewarning of any design issues.

[sunng87]

Sounds great! Do you have a link for the spec?

[weavejester]

Sure! https://github.com/ring-clojure/ring/blob/master/SPEC-alpha.md#3-websockets

[sunng87]

#116 is a working implementation for current ring.websocket spec

[weavejester]

Oh excellent, you beat me to the implementation! Thank you for your work on that.

[sunng87]

I have some feedback while implementing this:

• There is no on-ping for listener, but websocket allows client to issue ping packet
• Have you considered separated on-text and on-binary? The user may always need to instance? to check whether it's binary or text.
• There is an extensions field, like protocol, can be negotiated during handshake, you might want to add it to response map as well
• We may need some shortcuts to access protocols and extensions in request map as we now have first-class support for websocket
• A remote-addr function can be added to Socket to access its host/port information

[weavejester]

Thanks for the feedback! It's genuinely very helpful.

There is no on-ping for listener, but websocket allows client to issue ping packet

The on-ping event was omitted because the Jakarta EE MessageHandler class only allows for handling pong messages.

I wanted to create an implementation that could be used on a broad array of back ends without the user needing to worry about whether a particular feature was implemented, so I've gone for a "lowest common denominator" approach.

Have you considered separated on-text and on-binary?

Yes. The reason why they're not separated is because a Listener may be wrapped in a translation layer. For example:

(defn wrap-websocket-edn [listener]
  (reify Listener
    (on-message [_ sock mesg]
      (on-message listener
                  (wrap-socket-edn sock)
                  (edn/read-string mesg)))
    ...))

So rather than sending strings or bytes directly, you add a wrapper for translating edn, transit, json or whatever data format you want. You could even add middleware that checks the accepted protocol list on the request map, chooses a protocol, and automatically wraps the listener in the response in the appropriate translation layer.

So hypothetically, something like:

(defn handler [request]
  {::ws/listener
   {:on-message
    (fn [sock mesg] (ws/send! sock (update mesg :count inc)))}})

(def wrapped-handler
  (wrap-websocket-procotols handler {:edn edn-wrapper, :transit transit-wrapper}))

There is an extensions field, like protocol, can be negotiated during handshake, you might want to add it to response map as well.

Yes, I wasn't entirely sure how to go about testing this. My plan was to omit it for now, and if anyone complains add in a :ring.websocket/extensions field later.

We may need some shortcuts to access protocols and extensions in request map as we now have first-class support for websocket

There's a function request-protocols for accessing the protocol list on the request map. Is that the sort of thing you mean?

A remote-addr function can be added to Socket to access its host/port information

I don't I've run across any implementation where this wouldn't be accessible, so it's entirely possible to add it. But wouldn't this be the same information as :remote-addr on the request map?

[sunng87]

The on-ping event was omitted because the Jakarta EE MessageHandler class only allows for handling pong messages.

This might be a deal-breaker for some use-cases. My previous websocket application relies on client's ping for connection openness check. I would suggest to provide it as optional like send-async at least. The adapter can choose whether to implement it or not.

The reason why they're not separated is because a Listener may be wrapped in a translation layer.

Makes sense!

My plan was to omit it for now, and if anyone complains add in a :ring.websocket/extensions field later.

I am the first one :)

There's a function request-protocols for accessing the protocol list on the request map.

Yes! I was thinking about adding :websocket-protocols as an optional key to standard request map, like deprecated :content-type. But a helper function is sufficient for this case. And request-extensions is also needed when we are adding extensions to response map.

But wouldn't this be the same information as :remote-addr on the request map?

Yes but in this way we can only access it during handshaking. We will have to store it somewhere to be able it access it in listeners.

[weavejester]

This might be a deal-breaker for some use-cases. My previous websocket application relies on client's ping for connection openness check. I would suggest to provide it as optional like send-async at least. The adapter can choose whether to implement it or not.

That's interesting. It looks like Jakarta's design deliberately hides access to the ping event, considering it to be an implementation detail. I guess their view would be that if the server wants to check openness, it can send a ping itself and listen for the pong.

That said, we don't have to follow what Jakarta does. We could have a protocol as you describe:

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

Since you're the second person to comment on it, I'm inclined to add this. I can't see any compelling reason why not.

My plan was to omit it for now, and if anyone complains add in a :ring.websocket/extensions field later.

I am the first one :)

Are extensions something you've used, or know of someone who have used them? And if so, do you know of a Java websocket client library that supports checking for them? 🙂

Yes! I was thinking about adding :websocket-protocols as an optional key to standard request map, like deprecated :content-type. But a helper function is sufficient for this case. And request-extensions is also needed when we are adding extensions to response map.

Yep, I'll add a request-extensions function as well when I add extension support. The reason I prefer them as functions is then they're not calculated ahead of time, potentially unnecessarily, and there's only one place the information is accessed from (and one source of truth).

But wouldn't this be the same information as :remote-addr on the request map?

Yes but in this way we can only access it during handshaking. We will have to store it somewhere to be able it access it in listeners.

Very true, but the same thing applies to the URI and session information, right? My inclination is to lean toward having one place to find information, as it's a potentially slippery slope of duplicated data otherwise. Even if it is perhaps a little less convenient.

That said, it doesn't seem hard to pass the information via a closure:

(defn handler [{:keys [remote-addr]}]
  {::ws/listener {:on-open (fn [sock] (ws/send sock remote-addr))}})

[weavejester]

I've released Ring 1.11.0-alpha4 with a PingListener protocol, in order to support on-ping as you suggested.

[sunng87]

And if so, do you know of a Java websocket client library that supports checking for them? 🙂

I don't really use Jetty's websocket client in any of my projects but it seems to have support for extensions:

• https://github.com/eclipse/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-websocket/jetty-websocket-core-client/src/main/java/org/eclipse/jetty/websocket/core/client/CoreClientUpgradeRequest.java#L166

My inclination is to lean toward having one place to find information, as it's a potentially slippery slope of duplicated data otherwise. Even if it is perhaps a little less convenient.

I've changed my mind and agree with you. Let's keep the API set compact and small.

I've released Ring 1.11.0-alpha4 with a PingListener protocol, in order to support on-ping as you suggested.

PingListener has been adopted.

[weavejester]

You may want to check that the Ring websocket protocol implementations in rj9a written for the beta still work in the released version. There were a couple of changes made based on feedback.

[sunng87]

I have been following your development and it should work.

[weavejester]

I just looked over the code as a double-check, and it mostly looks great. But there's two errors that I think won't show until runtime.

The first is that -send-async is part of the AsyncSocket protocol, not the Socket protocol. I thought that Clojure would throw an error if you try to assign methods that aren't part of a protocol, but weirdly it doesn't! e.g.

(defprotocol Foo (foo [x] x)
(extend-protocol Foo String (foo [x] x) (bar [x] x))
;; no errors are thrown despite `bar` not being in `Foo`!

The second is more subtle: I changed the spec from using String to using the lower level CharSequence interface. This change was to allow for more performant classes like StringBuilder and StringBuffer to be used, if the user needed it.

With these two changes, I think the socket definition should be:

(extend-type Session
  ring-ws/Socket
  (-send [this msg]
    (if (instance? CharSequence msg)
      (.sendText this msg (write-callback noop noop))
      (.sendBinary this msg (write-callback noop noop))))
  (-ping [this msg]
    (.sendPing this msg (write-callback noop noop)))
  (-pong [this msg]
    (.sendPong this msg (write-callback noop noop)))
  (-close [this status-code reason]
    (.close this status-code reason (write-callback {})))
  (-open? [this]
    (.isOpen this))
  ring-ws/AsyncSocket
  (-send-async [this msg succeed fail]
    (if (instance? CharSequence msg)
      (.sendText this msg (write-callback succeed fail))
      (.sendBinary this msg (write-callback succeed fail)))))

[sunng87]

Great! I just created #121 for the fix