taoensso / sente

Realtime web comms library for Clojure/Script
https://www.taoensso.com/sente
Eclipse Public License 1.0
1.73k stars 193 forks source link

Add Jetty adapter #375

Closed wavejumper closed 3 years ago

wavejumper commented 3 years ago

While the official Jetty adapter for Ring does not support websockets, ring-jetty9-adapter does - which makes this adapter possible.

Fixes #371

I left some notes in the ns docstring, but here's an example using ring-defaults:

(require '[taoensso.sente :as sente])
(require '[taoensso.sente.server-adapters.jetty :as adapters.jetty])
(require '[ring.middleware.defaults :as ring.defaults])
(require '[ring.adapter.jetty9 :as jetty])

(def sente 
  (sente/make-channel-socket! (adapters.jetty/get-sch-adapter) {}))

(def defaults
  (-> ring.defaults/site-defaults
      (assoc-in [:responses :content-types] false)
      (assoc-in [:session :flash] false)))

(jetty/run-jetty handler {:port 3000
                          :websockets {"/chsk" (-> (:ajax-get-or-ws-handshake-fn sente)
                                                   (ring.defaults/wrap-defaults defaults))}
                          :allow-null-path-info true})

:ajax-post-fn would be configured as a regular route inside of handler

ptaoussanis commented 3 years ago

Awesome, thanks Thomas! Planning to work on Sente this week, will try merge + cut a new release by the weekend.

wavejumper commented 3 years ago

@ptaoussanis awesome, thanks!

And cheers for sente! I love the design, and how simple it was to build an adapter for Jetty. We were able to move our product from http-kit to jetty without any fuss 👍

ptaoussanis commented 3 years ago

Hi @wavejumper,

Could you please clarify what you mean by the following in the ns docstring?:

Note: ring-jetty9-adapter requires its own middleware stack for websocket connections. Thus, stateful Ring middleware require a little extra configuration: ring-session: pass shared instance of :store.

Do you have a source/reference for this somewhere? I couldn't find anything with a quick Google, and it's not obvious from your code where this might be relevant? Would like to understand better what's actually required of adapter consumers.

Thanks!

wavejumper commented 3 years ago

Hi @wavejumper,

Could you please clarify what you mean by the following in the ns docstring?:

Note: ring-jetty9-adapter requires its own middleware stack for websocket connections. Thus, stateful Ring middleware require a little extra configuration: ring-session: pass shared instance of :store.

Do you have a source/reference for this somewhere? I couldn't find anything with a quick Google, and it's not obvious from your code where this might be relevant? Would like to understand better what's actually required of adapter consumers.

Thanks!

Basically, ring-jetty9-adapter defines its websocket routes independent of your regular ring handler.

In my example above, note that the key :websockets has a map of WS paths -> {"/chsk" ...}

The caveat is that stateful ring middleware (like ring-session) can be instantiated twice.

This is relevant for sente as by default CSRF is enabled, and the default strategy for ring-anti-forgery is session based.

A comment made by the author of ring-jetty9-adapter demonstrates you can actually reuse the same handler: https://github.com/sunng87/ring-jetty9-adapter/issues/41#issuecomment-630206233

However, I actually prefer having two independent handlers - as I can configure my middleware stack for websockets to be tailored to API requests, and my regular ring handler tailored to site requests.

I guess the tl;dr is because there is no unified ring handler, it is easy to configure this Jetty adapter in a way that won't work (eg, you'll get Bad CSRF token responses).

More than happy to contribute an example project, or go into detail about usage in the README, just let me know.

ptaoussanis commented 3 years ago

Thanks for the extra info!

So if I understand correctly: the configuration that you're talking about here isn't specifically to do with your Jetty adapter - but is just general guidance regarding Jetty 9 and WebSockets?

If this is the case, then I'd prefer not to include too much documentation within Sente or your adapter (since it's a bit out of scope). We can maybe instead just point to a reference (e.g. ring-jetty9-adapter README + your attached link)?

wavejumper commented 3 years ago

So if I understand correctly: the configuration that you're talking about here isn't specifically to do with your Jetty adapter - but is just general guidance regarding Jetty 9 and WebSockets?

Correct - the docstring was my attempt to save someone a few hours of pain trying to setup their anti-forgery tokens :)

If this is the case, then I'd prefer not to include too much documentation within Sente or your adapter (since it's a bit out of scope). We can maybe instead just point to a reference (e.g. ring-jetty9-adapter README + your attached link)?

This sounds like a good idea, I'll update the ns docstring to point to a reference later tonight.

ptaoussanis commented 3 years ago

This sounds like a good idea, I'll update the ns docstring to point to a reference later tonight.

No worries, can do myself quickly - will get PR merged shortly.

Thanks again!

ptaoussanis commented 3 years ago

Merged manually. Added a link in the ns docstring to a "full example" Gist at https://gist.github.com/ptaoussanis/5aa2a55f8ca74cdfa77d66e8aa2c9b95.

@wavejumper Please feel free to fork + tweak/extend that example if you like, and I'll mod the link to point to your fork.

Thanks again for the PR, will be great to have Jetty support out-the-box!

wavejumper commented 3 years ago

Merged manually. Added a link in the ns docstring to a "full example" Gist at https://gist.github.com/ptaoussanis/5aa2a55f8ca74cdfa77d66e8aa2c9b95.

@wavejumper Please feel free to fork + tweak/extend that example if you like, and I'll mod the link to point to your fork.

Thanks again for the PR, will be great to have Jetty support out-the-box!

Brilliant, thanks!

I'll fork the gist and extend it to look like how we have Jetty + sente configured for the product I am working on, Operatr

wavejumper commented 3 years ago

@ptaoussanis here's the updated gist that I tested works locally against current master: https://gist.github.com/wavejumper/40c4cbb21d67e4415e20685710b68ea0