Skip to content

Close output channel on error #42

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

Merged
merged 2 commits into from
Oct 28, 2023
Merged

Close output channel on error #42

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bcc10b
Close output channel on error
ferdinand-beyer Oct 24, 2023
ad3952c
Use a `java.lang.Object` to test non-serialisable
ferdinand-beyer Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/lsp4clj/io_chan.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,10 @@
(with-open [writer output] ;; close output when channel closes
(loop []
(when-let [msg (async/<!! messages)]
(write-message writer msg)
(try
(write-message writer msg)
(catch Throwable e
(async/close! messages)
(throw e)))
Copy link
Member

@ericdallo ericdallo Oct 24, 2023

Choose a reason for hiding this comment

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

I suppose this throw is enough to the developer know this was a JSON parse exception or something right?

Copy link
Contributor Author

@ferdinand-beyer ferdinand-beyer Oct 25, 2023

Choose a reason for hiding this comment

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

I think it depends a bit on the setup. Because this runs within a core.async/thread, the exception is probably given to the thread's uncaught exception handler. I think the default one will dump the stack trace to stderr, but many logging frameworks will allow to install a custom handler to log those. Definitely something people should do.

As a library, I figured we should stay un-opinionated here and just throw.

Copy link
Member

@ericdallo ericdallo Oct 28, 2023

Choose a reason for hiding this comment

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

Yeah, agreed

(recur)))))
messages))
32 changes: 31 additions & 1 deletion test/lsp4clj/io_chan_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,34 @@
(:require
[clojure.core.async :as async]
[clojure.string :as string]
[clojure.test :refer [deftest is testing]]
[clojure.test :refer [deftest is testing use-fixtures]]
[lsp4clj.io-chan :as io-chan]
[lsp4clj.test-helper :as h]))

(set! *warn-on-reflection* true)

(defn- silence-uncaught-exceptions [f]
(let [handler (Thread/getDefaultUncaughtExceptionHandler)]
(Thread/setDefaultUncaughtExceptionHandler
(reify Thread$UncaughtExceptionHandler
(uncaughtException [_this _thread _e])))
(f)
(Thread/setDefaultUncaughtExceptionHandler handler)))

(use-fixtures :once silence-uncaught-exceptions)

(defn ^:private message-lines [arr]
(string/join "\r\n" arr))

(defn mock-input-stream [^String input]
(.getBytes input "utf-8"))

(defn error-output-stream []
(proxy [java.io.OutputStream] []
(close [] (throw (java.io.IOException. "close failed")))
(flush [] (throw (java.io.IOException. "flush failed")))
(write [& _] (throw (java.io.IOException. "write failed")))))

(deftest output-stream-should-camel-case-output
(let [output-stream (java.io.ByteArrayOutputStream.)
output-ch (io-chan/output-stream->output-chan output-stream)]
Expand Down Expand Up @@ -50,6 +66,20 @@
"{\"key\":\"äpfel\"}"])
(.toString output-stream "utf-8")))))

(deftest output-stream-error-should-close-output-channel
(testing "when JSON serialisation fails"
(let [output-stream (java.io.ByteArrayOutputStream.)
output-ch (io-chan/output-stream->output-chan output-stream)]
(async/>!! output-ch {:not-serializable output-stream})
(Thread/sleep 200)
(is (false? (async/put! output-ch {:test "should be closed"})))))
(testing "when an I/O exception occurs"
(let [output-stream (error-output-stream)
output-ch (io-chan/output-stream->output-chan output-stream)]
(async/>!! output-ch {:test "ok"})
(Thread/sleep 200)
(is (false? (async/put! output-ch {:test "should be closed"}))))))

(deftest input-stream-should-kebab-case-input
(let [input-stream (mock-input-stream
(message-lines
Expand Down