The "complete-doc" op lacks error handling

[sanjayl]

There's no exception handling around the complete-doc op in the cider.nrepl.middleware.completion middleware. The propagated exception hangs my environment.

Reproducible by passing in an empty string as the symbol (or leaving out that kv pair entirely), so probably just an in vitro situation:

(require '[cider.nrepl.test-session :as session])
(session/session-fixture #(session/message {:op "complete-doc" :symbol ""}))

Repeat of #180?

After having this fail in the cider-repl, the system seems to get pretty hangy; more so than when it blows up the test runner (although that might just be my imagination). The messages are attached below in case there is infact something weird.

Is there a way to pull out the *-reply from each middleware and standardize it with guaranteed error checking? Maybe something like (comms/reply [msg response & error]?? You'd pass in the msg, a closure to build the reply, and an optional closure to build a custom error packet, executed with a try/catch? Apologies if it already exists or is impossible; I'm extremely new to the codebase.

(---> 
  ns  "user"
  op  "eval"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  code  "(session/session-fixture #(session/message {:op \"complete-doc\" :symbol \"\"}))\n"
  file  "*cider-repl cider-nrepl*"
  line  4
  column  6
  id  "10"
)
(<- 
  err  "ERROR: Unhandled REPL handler exception processing message {:id 6b48f63c-8c85-480c-ba93-e34d47f7925c, :op complete-doc, :session 4a6a45e0-84fa-4469-8095-89242cebfb66, :symbol }\n"
  id  "10"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
)
(---> 
  op  "interrupt"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  interrupt-id  "8"
  id  "11"
)
(---> 
  op  "interrupt"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  interrupt-id  "7"
  id  "12"
)
(---> 
  op  "interrupt"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  interrupt-id  "10"
  id  "13"
)
(---> 
  op  "interrupt"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  interrupt-id  "5"
  id  "14"
)
(<- 
  id  "11"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("done" "interrupt-id-mismatch" "error")
)
(<- 
  id  "12"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("done" "interrupt-id-mismatch" "error")
)
(<- 
  id  "10"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("interrupted")
)
(<- 
  id  "13"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("done")
)
(<- 
  id  "14"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("done" "interrupt-id-mismatch" "error")
)
(<- 
  id  "10"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("done")
)
(<- 
  changed-namespaces  (dict)
  id  "10"
  repl-type  "clj"
  session  "aa478078-a963-43cd-a7c3-540bd5bd9ddc"
  status  ("state")
)

[bbatsov]

I tried this (complete {:ns "clojure.core" :sym ""}) and it doesn't blow up. A small mystery...

Is there a way to pull out the *-reply from each middleware and standardize it with guaranteed error checking? Maybe something like (comms/reply [msg response & error]?? You'd pass in the msg, a closure to build the reply, and an optional closure to build a custom error packet, executed with a try/catch? Apologies if it already exists or is impossible; I'm extremely new to the codebase.

It should be doable, but we haven't gotten there yet.

[sanjayl]

Give (completion-doc {:ns "clojure.core" :sym ""}) a try instead of (complete {:ns "clojure.core" :sym ""})...but you might want to throw a try/catch around it first.

It looks like the problem is deeper down in the plumbing, so I'll take the underlying issue elsewhere. Apparently you can have a ghost symbol for lack of a better term. i.e., you can make a symbol out of the empty string??

user=> (symbol "")

user=> (symbol? (symbol ""))
true
user=> 

Unfortunately, you can never even attempt to resolve such a symbol (which compliment.core/documentation tries to do, causing the exception):

user=> (ns-resolve *ns* (symbol ""))
StringIndexOutOfBoundsException String index out of range: 0  java.lang.String.charAt (String.java:658)

I'm guessing this should return nil instead.

I'm currently trying to write coverage tests, but I'd be happy to try abstracting-out the messaging once that's further along.

[bbatsov]

Btw, there's error handling:

(defn complete-reply
  [{:keys [transport] :as msg}]
  (try
    (transport/send
     transport
     (response-for msg :completions (complete msg) :status :done))
    (catch Exception e
      (transport/send
       transport
       (response-for msg (u/err-info e :completion-error))))))

[bbatsov]

Ah, completion-doc... Sorry about this. I never use this directly and I forgot we had it. :-)

[bbatsov]

I'm guessing this should return nil instead.

Definitely. @alexander-yakushev I think that ideally this should be fixed in compliment itself.

I'm currently trying to write coverage tests, but I'd be happy to try abstracting-out the messaging once that's further along.

That'd be great. :-)

[bbatsov]

P.S. Actually documentation should ideally return "", but the important thing is that it shouldn't blow up.