Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for serving on Unix domain socket #478

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add support for serving on Unix domain socket #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7e22d4
add support for running unix domain socket connector
hsartoris-bard Apr 7, 2023
7a0ff20
rename option to :unix-socket; add check for windows
hsartoris-bard Apr 10, 2023
c0aec47
make HTTP default false when :unix-socket is specified
hsartoris-bard Apr 10, 2023
9ede539
basic socket server test
hsartoris-bard Apr 10, 2023
71ec504
error on extant file; add doc line
hsartoris-bard Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions ring-jetty-adapter/project.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@
:dependencies [[org.clojure/clojure "1.7.0"]
[ring/ring-core "1.10.0"]
[ring/ring-servlet "1.10.0"]
[org.eclipse.jetty/jetty-server "9.4.51.v20230217"]]
[org.eclipse.jetty/jetty-server "9.4.51.v20230217"]
[org.eclipse.jetty/jetty-unixsocket "9.4.51.v20230217"]]
:aliases {"test-all" ["with-profile" "default:+1.8:+1.9:+1.10:+1.11" "test"]}
:profiles
{:dev {:dependencies [[clj-http "3.12.3"]
[less-awful-ssl "1.0.6"]]
[less-awful-ssl "1.0.6"]
[io.projectreactor.netty/reactor-netty "1.1.5"]]
:jvm-opts ["-Dorg.eclipse.jetty.server.HttpChannelState.DEFAULT_TIMEOUT=500"]}
:1.8 {:dependencies [[org.clojure/clojure "1.8.0"]]}
:1.9 {:dependencies [[org.clojure/clojure "1.9.0"]]}
Expand Down
37 changes: 34 additions & 3 deletions ring-jetty-adapter/src/ring/adapter/jetty.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
"A Ring adapter that uses the Jetty 9 embedded web server.

Adapters are used to convert Ring handlers into running web servers."
(:require [ring.util.servlet :as servlet])
(:require [clojure.java.io :as io]
[ring.util.servlet :as servlet])
(:import [org.eclipse.jetty.server
Request
Server
Expand All @@ -13,11 +14,13 @@
SslConnectionFactory
SecureRequestCustomizer]
[org.eclipse.jetty.server.handler AbstractHandler]
[org.eclipse.jetty.unixsocket UnixSocketConnector]
[org.eclipse.jetty.util BlockingArrayQueue]
[org.eclipse.jetty.util.thread ThreadPool QueuedThreadPool]
[org.eclipse.jetty.util.ssl SslContextFactory$Server KeyStoreScanner]
[javax.servlet AsyncContext DispatcherType AsyncEvent AsyncListener]
[javax.servlet.http HttpServletRequest HttpServletResponse]))
(set! *warn-on-reflection* true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this line, please.


(defn- ^AbstractHandler proxy-handler [handler]
(proxy [AbstractHandler] []
Expand Down Expand Up @@ -130,6 +133,25 @@
(.setHost (options :host))
(.setIdleTimeout (options :max-idle-time 200000)))))

(defn- socket-connector
^UnixSocketConnector [^Server server {:keys [unix-socket] :as options}]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a consistent format in this file, so:

(defn- ^UnixSocketConnector unix-socket-connector
  [^Server server {:keys [unix-socket] :as options}]

Also I think unix-socket-connector would help differentiate it from TCP sockets.

(when (->> (System/getProperty "os.name")
(re-find #"(?i)^windows"))
(throw (ex-info "Unix sockets not supported on windows"
{:os (System/getProperty "os.name")
:unix-socket unix-socket})))
Comment on lines +138 to +142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's factor this out into its own function (and also capitalize Windows):

(defn- check-os-socket-compatibility []
  (when (->> (System/getProperty "os.name")
             (re-find #"(?i)^windows"))
    (throw (ex-info "Unix sockets not supported on Windows"
                    {:os (System/getProperty "os.name")
                     :unix-socket unix-socket}))))

(let [http-factory (HttpConnectionFactory. (http-config options))
socket (io/file unix-socket)]
(when (.exists socket)
(throw (ex-info "File already exists at socket path; should be deleted before starting server"
{:unix-socket unix-socket})))
Comment on lines +145 to +147
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's factor this out as well:

(defn- check-socket-file-does-not-exist [socket]
  (when (.exists socket)
    (throw (ex-info "Could not create socket file: file already exists"
                    {:socket-file (str socket)}))))

(.deleteOnExit socket)
(doto (UnixSocketConnector.
server
#^"[Lorg.eclipse.jetty.server.ConnectionFactory;" (into-array ConnectionFactory [http-factory]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you split the line over 80 characters into two lines? Also can you fix the indentation so it's according to the style guide:

(doto (UnixSocketConnector.
       server
       ^"[Lorg.eclipse.jetty.server.ConnectionFactory;"
       (into-array ConnectionFactory [http-factory]))

(.setUnixSocket (.getAbsolutePath socket))
(.setIdleTimeout (options :max-idle-time 200000)))))

(defn- ^ThreadPool create-threadpool [options]
(let [min-threads (options :min-threads 8)
max-threads (options :max-threads 50)
Expand All @@ -150,10 +172,15 @@
(defn- ^Server create-server [options]
(let [pool (or (:thread-pool options) (create-threadpool options))
server (Server. pool)]
(when (:http? options true)
(when (:http? options (or ;; default is enabled when no socket specified
(not (:unix-socket options))
;; enabled with socket when host/port set
(some options [:host :port])))
Comment on lines +175 to +178
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels too complex, and isn't what the documentation says. I think let's keep it simple and just leave it defaulting to true. Let's add a note to the :unix-socket option instead to tell people they may want to set :http? to false.

(.addConnector server (http-connector server options)))
(when (or (options :ssl?) (options :ssl-port))
(.addConnector server (ssl-connector server options)))
(when (:unix-socket options)
(.addConnector server (socket-connector server options)))
server))

(defn ^Server run-jetty
Expand All @@ -170,10 +197,14 @@
:join? - blocks the thread until server ends
(defaults to true)
:daemon? - use daemon threads (defaults to false)
:http? - listen on :port for HTTP traffic (defaults to true)
:http? - listen on :port for HTTP traffic (defaults to true
unless :unix-socket is set)
:ssl? - allow connections over HTTPS
:ssl-port - the SSL port to listen on (defaults to 443, implies
:ssl? is true)
:unix-socket - File to be used as a Unix domain socket; will be
passed to [io/file]. Ensure there is no file at
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think [io/file] should be a link, as there's nothing to link it to.

this location when starting the server.
:ssl-context - an optional SSLContext to use for SSL connections
:exclude-ciphers - when :ssl? is true, additionally exclude these
cipher suites
Expand Down
25 changes: 23 additions & 2 deletions ring-jetty-adapter/test/ring/adapter/test/jetty.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,12 @@
[java.net ServerSocket ConnectException]
[java.security KeyStore]
[java.io SequenceInputStream ByteArrayInputStream InputStream
IOException]
[org.apache.http MalformedChunkCodingException]))
IOException
File]
Comment on lines +15 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines should be one.

[java.util.function Supplier]
[org.apache.http MalformedChunkCodingException]
[io.netty.channel.unix DomainSocketAddress]
[reactor.netty.http.client HttpClient]))

(defn- hello-world [request]
{:status 200
Expand Down Expand Up @@ -98,6 +102,23 @@
"text/plain"))
(is (= (:body response) "Hello World")))))

(testing "socket server"
(let [sock (File/createTempFile "ring-jetty-server-" ".sock")]
(.delete sock)
(with-server hello-world {:unix-socket sock}
(is (= (-> (HttpClient/create)
(.remoteAddress
(reify Supplier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off here.

(get [_]
(DomainSocketAddress. (.getAbsolutePath sock)))))
.get (.uri "/") .responseContent .aggregate .asString .block)
"Hello World")
"able to serve basic request")
(is (thrown-with-msg?
clojure.lang.ExceptionInfo #"File already exists"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation looks off. Please see Clojure style guide.

(run-jetty hello-world {:unix-socket sock, :join? false}))
"starting a new server on existing socket should fail"))))

(testing "HTTPS server"
(with-server hello-world {:port test-port
:ssl-port test-ssl-port
Expand Down