sunng87 / ring-jetty9-adapter

An enhanced version of jetty adapter for ring, with additional features like websockets, http/2 and http/3
Eclipse Public License 1.0
272 stars 48 forks source link

[Question] Requesting assistance adding support for ring-jetty9-adapter to Sente #89

Closed ptaoussanis closed 1 year ago

ptaoussanis commented 1 year ago

Hi Ning, thanks a lot for all your work on ring-jetty9-adapter!

Context

I maintain a Clojure/Script library called Sente. It provides an API to help make it easier for web apps to use WebSockets for client<->server communication.

Sente currently supports a number of Clojure web servers, incl. http-kit and ring-undertow-adapter.

Sente also previously supported ring-jetty9-adapter, but support seems to have been broken sometime around https://github.com/sunng87/ring-jetty9-adapter/pull/59.

I'm currently working on a major upcoming update to Sente (v1.18), and it'd be great to get ring-jetty9-adapter support working again.

The trouble is that I've not yet had the opportunity to use ring-jetty9-adapter myself - and I haven't been able to find the info I need about the API online.

Would you maybe be available to assist with this?

Details of what's needed

To support a web-server like ring-jetty9-adapter, Sente needs an "adapter" that implements these 2 protocols.

Example implementations:

This is currently what I have:

(ns taoensso.sente.server-adapters.jetty9
  "Sente adapter for ring-jetty9-adapter,
  (https://github.com/sunng87/ring-jetty9-adapter)."
  {:author "Thomas Crowley (@wavejumper), Jian Zhang (@surferHalo)"}
  (:require
   [clojure.string :as str]
   [ring.adapter.jetty9.websocket :as jetty9.websocket]
   [ring.adapter.jetty9 :as jetty]
   [taoensso.sente.interfaces :as i]))

(defn- ajax-cbs [sch]
  {:write-failed  (fn [_throwable] (jetty9.websocket/close! sch))
   :write-success (fn [          ] (jetty9.websocket/close! sch))})

(extend-type org.eclipse.jetty.websocket.api.WebSocketAdapter
  i/IServerChan
  (sch-open?  [sch] (jetty9.websocket/connected? sch))
  (sch-close! [sch] (jetty9.websocket/close!     sch))
  (sch-send!  [sch websocket? msg]
    (if websocket?
      (jetty9.websocket/send! sch msg)

      ;; @sunng87 This looks strange to me - is it correct that a
      ;; non-websocket send would use the websocket namespace?
      (jetty9.websocket/send! sch msg (ajax-cbs sch)))

    ;; @sunng87 This `sch-send!` method should return truthy iff it
    ;; believes the send has succeeded. Is there a better way of
    ;; of getting the return value?
    (i/sch-open? sch)))

(defn- server-ch-resp
  [websocket? {:keys [on-open on-close on-msg on-error]}]

  ;; @sunng87 This function may be called with both websocket
  ;; and non-websocket requests (`websocket?` may be true or false).
  ;;
  ;; The below code seems to not work unless `websocket?` is true,
  ;; but I couldn't find examples online of how to handle this case.

  ;; Return response to Ring request that `ring-jetty9-adapter` will understand:
  (jetty/ws-upgrade-response
    {:on-connect (fn [sch]          (on-open  sch websocket?))
     :on-text    (fn [sch msg]      (on-msg   sch websocket? msg))
     :on-error   (fn [sch error]    (on-error sch websocket? error))
     :on-close   (fn [sch status _] (on-close sch websocket? status))}))

(deftype JettyServerChanAdapter []
  i/IServerChanAdapter
  (ring-req->server-ch-resp [_ ring-req callbacks-map]
    (server-ch-resp (jetty/ws-upgrade-request? ring-req)
      callbacks-map)))

(defn get-sch-adapter [] (JettyServerChanAdapter.))

Any assistance/advice would be very much appreciated, thanks!

Cheers :-)

sunng87 commented 1 year ago

hi @ptaoussanis sorry for late response being pretty busy during weekdays. I will experiment the upgrade by myself and come back to you if there is any updates/questions.

ptaoussanis commented 1 year ago

No problem at all @sunng87, and thank you so much 🙏 Please take the time you need.

sunng87 commented 1 year ago

I had a quick look into original jetty adapter, we will need to change ring-req->server-ch-resp to:

(deftype JettyServerChanAdapter []
  i/IServerChanAdapter
  (ring-req->server-ch-resp [_ req callbacks-map]
    (jetty/ws-upgrade-response
     (server-ch-resp (jetty/ws-upgrade-request? req) callbacks-map))))

Just wrap the callbacks with jetty/ws-upgrade-response and it will take care of the upgrade handshake.

And for sch-send! implementation, I'm afraid we cannot use (ajax-cbs sch) here. The callback is Jetty's websocket write callback and it has nothing to do with our ajax implementation. We can get the result of write, our return value for sch-send! via this callback. However it's an async approach that may not fit into our current api.

I haven't been able to figure out how we can fallback to ajax when the request is not a websocket. We should be able to use the else branch of (jetty/ws-upgrade-request? req). But since the ajax fallback is not websocket I'm not sure if we can get a valid sch for ajax.

ptaoussanis commented 1 year ago

Hi @sunng87, thanks a lot for taking a look at this!

However it's an async approach that may not fit into our current api. [...] I haven't been able to figure out how we can fallback to ajax when the request is not a websocket.

Can ring-jetty9-adapter handle a normal (non-WebSocket) HTTP request so that the application can use a callback or some other way to asynchronously reply to the request when ready?

For example with http-kit:

(defn handle-ring-req [ring-req]
  (let [channel (org.httpkit.server/as-channel ring-req {})]

    (future
      ;; Later, when application is ready ...
      (Thread/sleep 2000)
      (org.httpkit.server/send! channel
        {:status  200
         :headers {"Content-Type" "text/plain"}
         :body    "Reply"}))

    channel))

I think Immutant and Undertow also work like this.

Can ring-jetty9-adapter also do something like this? It would be okay if the way to do it (API) looked different, I'd just need to see an example.

Or is this kind of async approach just fundamentally not currently possible?

Thank you!

sunng87 commented 1 year ago

it seems possible with ring's async api, check the async example in https://github.com/sunng87/ring-jetty9-adapter/blob/master/examples/rj9a/async.clj

for websocket, it's like:

(defn async-handler [request send-response _]
  (send-response
   (if (jetty/ws-upgrade-request? request)
     ;; if the request is a websocket upgrade, finish handshake
     (jetty/ws-upgrade-response my-websocket-handler)
     ;; otherwise, use send-response with a ring response map
     {:status 200 :body "hello"})))

We can create a channel implementation based on send-response.

By the way, if you are using async handler, add option :async? true when run-jetty

ptaoussanis commented 1 year ago

I see, thank you!

So if I understand correctly, it seems the only way to currently provide an async HTTP response via ring-jetty9-adapter would be through Ring's 3-arity async handler API?

I'll experiment with adding support for Ring's 3-arity async handlers to the next version of Sente 👍

This issue can be closed from my side, thanks again!

sunng87 commented 1 year ago

Thank you @ptaoussanis ! Let me know if you have any further questions!