Merge pull request #40 from clojure-lsp/deprecate-socket-server

Deprecate lsp4clj.socket-server

Author: ericdallo

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Deprecate `lsp4clj.socket-server` and document preferred alternative in the README.
+
 ## v1.7.3
 
 ## v1.7.2

README.md

@@ -4,7 +4,7 @@ A [Language Server Protocol](https://microsoft.github.io/language-server-protoco
 
 [![Clojars Project](https://img.shields.io/clojars/v/com.github.clojure-lsp/lsp4clj.svg)](https://clojars.org/com.github.clojure-lsp/lsp4clj)
 
-lsp4clj reads and writes from stdio, parsing JSON-RPC according to the LSP spec. It provides tools to allow server implementors to receive, process, and respond to any of the methods defined in the LSP spec, and to send their own requests and notifications to clients.
+lsp4clj reads and writes from io streams, parsing JSON-RPC according to the LSP spec. It provides tools to allow server implementors to receive, process, and respond to any of the methods defined in the LSP spec, and to send their own requests and notifications to clients.
 
 ## Usage
 
@@ -119,15 +119,25 @@ First, if the server's input is closed, it will shut down too. Second, if you ca
 
 When a server shuts down it stops reading input, finishes processing the messages it has in flight, and then closes is output. Finally it closes its `:log-ch` and `:trace-ch`. As such, it should probably not be shut down until the LSP `exit` notification (as opposed to the `shutdown` request) to ensure all messages are received. `lsp4clj.server/shutdown` will not return until all messages have been processed, or until 10 seconds have passed, whichever happens sooner. It will return `:done` in the first case and `:timeout` in the second.
 
-### Socket server
+## Other types of servers
 
-The `stdio-server` is the most commonly used, but the library also provides a `lsp4clj.socket-server/server`.
+So far the examples have focused on `lsp4clj.io-server/stdio-server`, because many clients communicate over stdio by default. The client opens a subprocess for the LSP server, then starts sending messages to the process via the process's stdin and reading messages from it on its stdout.
+
+Many clients can also communicate over a socket. Typically the client starts a socket server, then passes a command-line argument to the LSP subprocess, telling it what port to connect to. The server is expected to connect to that port and use it to send and receive messages. In lsp4clj, that can be accomplished with `lsp4clj.io-server/server`:
 
 ```clojure
-(lsp4clj.socket-server/server {:port 61235})
+(defn socket-server [{:keys [host port]}]
+  {:pre [(or (nil? host) (string? host))
+         (and (int? port) (<= 1024 port 65535))]}
+  (let [addr (java.net.InetAddress/getByName host) ;; nil host == loopback
+        sock (java.net.Socket. ^java.net.InetAddress addr ^int port)]
+    (lsp4clj.io-server/server {:in sock
+                               :out sock})))
 ```
 
-This will start listening on the provided port, blocking until a client makes a connection. When the connection is made it returns a lsp4clj server that has the same behavior as a `stdio-server`, except that messages are exchanged over the socket. When the server is shut down, the connection will be closed.
+`lsp4clj.io-server/server` accepts a pair of options `:in` and `:out`. These will be coerced to a `java.io.InputStream` and `java.io.OutputStream` via `clojure.java.io/input-stream` and `clojure.java.io/output-stream`, respectively. The example above works because a `java.net.Socket` can be coerced to both an input and output stream via this mechanism.
+
+A similar approach can be used to connect over pipes.
 
 ## Development details
 
@@ -156,7 +166,11 @@ You may also find `lsp4clj.server/chan-server` a useful alternative to `stdio-se
 
 ## Caveats
 
-You must not print to stdout while a `stdio-server` is running. This will corrupt its output stream and clients will receive malformed messages. To protect a block of code from writing to stdout, wrap it with `lsp4clj.server/discarding-stdout`. The `receive-notification` and `receive-request` multimethods are already protected this way, but tasks started outside of these multimethods need this protection added. Consider using a `lsp4clj.socket-server/server` to avoid this problem.
+You must not print to stdout while a `stdio-server` is running. This will corrupt its output stream. Clients will receive malformed messages, and either throw errors or stop responding.
+
+From experience, it's dismayingly easy to leave in an errant `prn` or `time` and end up with a non-responsive client. For this reason, we highly recommend supporting communication over sockets (see [other types of servers](#other-types-of-servers)) which are immune to this problem. However, since the choice of whether to use sockets or stdio is ultimately up to the client, you may have no choice but to support both.
+
+lsp4clj provides one tool to avoid accidental writes to stdout (or rather to `*out*`, which is usually the same as `System.out`). To protect a block of code from writing to `*out*`, wrap it with `lsp4clj.server/discarding-stdout`. The `receive-notification` and `receive-request` multimethods are already protected this way, but tasks started outside of these multimethods or that run in separate threads need this protection added.
 
 ## Known lsp4clj users
 

src/lsp4clj/io_chan.clj

@@ -7,7 +7,11 @@
    [clojure.java.io :as io]
    [clojure.string :as string])
   (:import
-   (java.io EOFException InputStream OutputStream)))
+   (java.io
+    EOFException
+    IOException
+    InputStream
+    OutputStream)))
 
 (set! *warn-on-reflection* true)
 
@@ -72,15 +76,18 @@
 (defn ^:private read-header-line
   "Reads a line of input. Blocks if there are no messages on the input."
   [^InputStream input]
-  (let [s (java.lang.StringBuilder.)]
-    (loop []
-      (let [b (.read input)] ;; blocks, presumably waiting for next message
-        (case b
-          -1 ::eof ;; end of stream
-          #_lf 10 (str s) ;; finished reading line
-          #_cr 13 (recur) ;; ignore carriage returns
-          (do (.append s (char b)) ;; byte == char because header is in US-ASCII
-              (recur)))))))
+  (try
+    (let [s (java.lang.StringBuilder.)]
+      (loop []
+        (let [b (.read input)] ;; blocks, presumably waiting for next message
+          (case b
+            -1 ::eof ;; end of stream
+            #_lf 10 (str s) ;; finished reading line
+            #_cr 13 (recur) ;; ignore carriage returns
+            (do (.append s (char b)) ;; byte == char because header is in US-ASCII
+                (recur))))))
+    (catch IOException _e
+      ::eof)))
 
 (defn input-stream->input-chan
   "Returns a channel which will yield parsed messages that have been read off

src/lsp4clj/socket_server.clj

@@ -1,4 +1,22 @@
 (ns lsp4clj.socket-server
+  "DEPRECATED: Start a socket and listen on it.
+
+  This namespace is deprecated and is scheduled for deletion. See the README for
+  recommendations on how to communicate with an LSP client over a socket.
+
+  This namespace was implemented based on a misconception. The LSP spec says
+  servers should accept a `--port` CLI argument for socket-based communication.
+  We assumed that the server was expected to start a socket server on that port,
+  then wait for the client to connect and start sending messages. However, this
+  is not how clients actually work. Instead, the _client_ opens the socket
+  server, and tells the LSP server which port to connect to. That is, the LSP
+  client acts as a socket server and the LSP server acts as a socket client.
+
+  This namespace, which largely deals with starting a socket server, is
+  therefore unusable in an LSP server. If the LSP server did try to open a
+  server on the provided `--port`, it should always fail, because the client
+  should already be listening on that port."
+  {:deprecated "1.7.4"}
   (:require
    [lsp4clj.io-server :as io-server])
   (:import

test/lsp4clj/io_server_test.clj

@@ -6,18 +6,21 @@
    [lsp4clj.io-server :as io-server]
    [lsp4clj.lsp.requests :as lsp.requests]
    [lsp4clj.server :as server]
-   [lsp4clj.test-helper :as h]))
+   [lsp4clj.test-helper :as h])
+  (:import
+   [java.io PipedInputStream PipedOutputStream]
+   [java.net InetAddress ServerSocket Socket]))
 
 (set! *warn-on-reflection* true)
 
-(deftest server-test
-  (let [server-input-stream (java.io.PipedInputStream.)
-        server-output-stream (java.io.PipedOutputStream.)
-        client-input-stream (java.io.PipedInputStream. server-output-stream)
-        client-output-stream (java.io.PipedOutputStream. server-input-stream)
-        server (io-server/server {:in server-input-stream :out server-output-stream})
+(deftest should-communicate-over-io-streams
+  (let [client-input-stream (PipedInputStream.)
+        client-output-stream (PipedOutputStream.)
+        server-input-stream (PipedInputStream. client-output-stream)
+        server-output-stream (PipedOutputStream. client-input-stream)
         client-input-ch (io-chan/input-stream->input-chan client-input-stream)
         client-output-ch (io-chan/output-stream->output-chan client-output-stream)
+        server (io-server/server {:in server-input-stream :out server-output-stream})
         join (server/start server nil)]
     ;; client initiates request
     (async/put! client-output-ch (lsp.requests/request 1 "foo" {}))
@@ -37,3 +40,30 @@
            (h/assert-take client-input-ch)))
     (server/shutdown server)
     (is (= :done @join))))
+
+(deftest should-communicate-through-socket
+  (let [addr (InetAddress/getLoopbackAddress)
+        ;; ephermeral port, translated to real port via .getLocalPort
+        port 0]
+    (with-open [socket-server (ServerSocket. port 0 addr)
+                socket-for-server (Socket. addr (.getLocalPort socket-server))
+                socket-for-client (.accept socket-server)]
+      (let [client-input-ch (io-chan/input-stream->input-chan socket-for-client)
+            client-output-ch (io-chan/output-stream->output-chan socket-for-client)
+            server (io-server/server {:in socket-for-server
+                                      :out socket-for-server})
+            join (server/start server nil)]
+        (async/put! client-output-ch (lsp.requests/request 1 "foo" {}))
+        (is (= {:jsonrpc "2.0",
+                :id 1,
+                :error {:code -32601,
+                        :message "Method not found",
+                        :data {:method "foo"}}}
+               (h/assert-take client-input-ch)))
+        (server/send-notification server "bar" {:key "value"})
+        (is (= {:jsonrpc "2.0",
+                :method "bar",
+                :params {:key "value"}}
+               (h/assert-take client-input-ch)))
+        (server/shutdown server)
+        (is (= :done @join))))))