Exception handling

[carocad]

General notes on the implementation, features and changes

• This PR introduces inline-collapsible-exceptions by parsing the stacktrace and highlighting/hyperlinking to the relevant lines of code that caused the problem
  • Only the current project files can have hyperlinks/highlights given that clojure and other libraries are (usually) compressed jars to which we don't have an absolute path before decompressing them.
• This PR changes the way ResultHandler() functions get their values on two points. msg.error is replaced by msg which has an err property by default. A new type of message msg.ex is also accepted as a valid response for the resultHandlers.
• This PR changes the point at which messages to the repl are received. Before, we listened for every messages sent by the repl, now we wait for all messages to arrive and then send them to the repl. (streams vs bulk processing)
  • The intention of this is to avoid sending to the repl the messages to stderr that match a stacktrace pattern.
• This PR also introduces two new files with the intention of simplifying/encapsulating two different things.
  • inline-result: The value.coffee file can be extended to support new and customized ways to display different things/values inline. Supporting canvas/plots inline, docs and many more should be as easy as extending the render function to support a new case. (docs are already implemented)
  • Stacktraces post-processing: the stacktraces.coffee file can be extended to support new stacktraces regex patterns produced by exception-handling libraries other than clojure.repl. The idea is to be able to define and use a new regex pattern without having to modify any other post-processing code

I hope that you and other clojure users would like this :)

[jasongilman]

Thanks again for continuing to work on this! Can you fix the merge conflicts and make the renaming suggestion I put in the issue a few minutes ago? I'm not quite ready to dig into this pull request yet but I plan to get to it soon.

[carocad]

@jasongilman good to go :)

Don't worry about not knowing yet what I did. Take your time ;)

I think it is very important (specially for you) to understand all of my changes and the reasons behind them, after all, people submit their issue to you.

If you need to ask me something just pin me on slack or on the issue itself and I will try to explain things as good as I can.

[jasongilman]

@carocad I tried it out and it's awesome. I'd really like to get this feature in Proto REPL. The highlighting is more useful than I thought it would be.

Some notes:

• It would be nice to get the stack trace without having to run pst. I think the best way to do that would be to detect if an exception was returned at the REPL. I noticed that a message from nREPL is returned with an ex and root-ex key. We could detect that and automatically run pst.
  
• The link clicking isn't working right now. It moves the mouse pointer but not to the expected location. It seems like it goes to the last location where the execution was initiated.

[carocad]

@jasongilman cool :)

I'm glad you like it. I also think that highlighting and hyperlinks are awesome !

Regarding your notes:

• Thanks for letting me know about the hyperlinks not working. I pushed a commit with that fixed. It seem I didn't properly test it after I updated from html objects to space-pen objects. It should work now.
• Yes, automatically getting the stacktrace when we detect an exception should definitely be our next step/release?. However there are some things which are currently preventing us from doing so:
  • From my tests I discovered that when an exception happens, we first receive an eval-error message containing only the exception type. Later on, we receive the previous message AND the rest of the output from the nrepl (thus we receive a duplicated message).
    If we were to only check for ex and root-ex we would have to ask the nrepl twice for the latest exception, which also means twice parsing, highlighting and hyperlinks. Assuming that our code doesn't throw any exception that would overwrite *e proto-repl would still have to do quite some work due to that bug. There is currently a PR on the node-nrepl-client regarding that: Use done status to mark end of message seq rksm/node-nrepl-client#7. I haven't tested it but it is precisely about the bug that's I'm talking about.
  • As I mentioned on the issue description. If we automatically run repl/pst on an error, we would be forcing the user to use our choice of stacktrace-parsing library. I would totally disagree with that as I think that should be a personal choice. Sadly, I don't have yet a compelling strategy to tackle the problem of automatic getting the stacktrace but I think maybe (just maybe) putting a user setting to indicate which stacktrace library to use in order to get the stacktrace and based on that choice, send a custom code to require and call the exception-parsing function which would return the stacktrace under that specific library.

In any case, I would first like to get this feature out, get some user feedback and implementing other stacktrace parsers and then check how to implement the automatic & inline stacktrace. What do you think?

[jasongilman]

I wrote up a bunch of stuff in response but my computer crashed so I'll attempt to recreate it.

We can work around the nREPL bug. There are multiple ways to do that. One method I thought of was with a javascript timeout. We start a timeout (very short) on seeing the indication of an exception. We also clear any existing exception timeout when we do that. If we use that approach it will suppress running the code multiple times and only run the parsing and displaying on the last run.

Can clojure.repl/pst be overridden by other libraries? I was under the impression that they add their own version of that function but it's not in the clojure.repl namespace. It might be best in any case to use something else. We can control the code that's generating the stack trace if we're the one calling it and we don't have to parse anything. We can call a function other than pst that returns a stacktrace data structure. The function can be put in the proto-repl Clojure library. I've pasted an example below with some sample output.

I'd prefer not to merge it in the current state where it changes how output is parsed. I think the minimum version should handle automatic printing of stack traces from code that generates an exception. I can understand that you've been working on this one a while and may be getting tired of continuing to fix things in it. It's so close now. I wouldn't mind taking over from here if you'd prefer me to figure out the best way to merge it in. I'll let you decide though. What you've created is awesome. If we go a little bit farther then I think it will have the best interaction and utility for the user.

Example code for returning a stack trace data structure. This is based on code in clojure.repl

(defn stack-frame-to-details
  [^StackTraceElement el]
  (let [file (.getFileName el)
        clojure-fn? (and file (or (.endsWith file ".clj")
                                  (.endsWith file ".cljc")
                                  (= file "NO_SOURCE_FILE")))]
    (if clojure-fn?
      [(clojure.repl/demunge (.getClassName el))
       (.getFileName el)
       (.getLineNumber el)]
      [(str (.getClassName el) "." (.getMethodName el))
       (.getFileName el)
       (.getLineNumber el)])))

(defn exception-to-stack-details
  ([]
   (exception-to-stack-details *e))
  ([^Throwable e]
   (for [el (take 50
                  (remove #(#{"clojure.lang.RestFn" "clojure.lang.AFn"} (.getClassName %))
                          (.getStackTrace e)))]
     (stack-frame-to-details el))))

Sample output

(["clojure.lang.Numbers.divide" "Numbers.java" 158]
 ["clojure.lang.Numbers.divide" "Numbers.java" 3808]
 ["proto-repl-demo.demo/add-numbers" "demo.clj" 8]
 ["proto-repl-demo.demo/add-numbers" "demo.clj" 5]
 ["proto-repl-demo.demo/eval10643"
  "form-init2165996922286074120.clj"
  50]
 ["proto-repl-demo.demo/eval10643"
  "form-init2165996922286074120.clj"
  50]
 ["clojure.lang.Compiler.eval" "Compiler.java" 6927]
 ["clojure.lang.Compiler.eval" "Compiler.java" 6890]
...

[carocad]

hey @jasongilman I will try to put my thoughts on this as clear as possible but please be aware that this is a big and somehow philosophical matter as this is no longer about software but rather usability opinions. Also I'm not 100% sure that I have understood everything that you mean so ... hard to give proper feedback like that.

• I would still like this feature to go out like this, not because I have worked on it a long time but rather because I think it should be the users who 'decide' which should be the next step. I think publishing it as it is would give us very good feedback on whether or not do they find it problematic to have the stacktraces only on pst or if they want silver bullet solution. I think we should listen to them first and then decide based on that.

Can clojure.repl/pst be overridden by other libraries? I was under the impression that they add their own version of that function but it's not in the clojure.repl namespace. It might be best in any case to use something else.

• Yes they can. Even with the introduction of direct linking in Clojure 1.8, repl/pst can and is still overwritten by most libraries. So as long as a library redefines repl/pst we could get the stacktrace via it.

We can control the code that's generating the stack trace if we're the one calling it and we don't have to parse anything. We can call a function other than pst that returns a stacktrace data structure. The function can be put in the proto-repl Clojure library. I've pasted an example below with some sample output.

• This is where we disagree. I really like that, so far, I didn't have to modify any of my projects to use proto-repl capabilities. It just works out of the box without having to set up thousand of little things, like most other tools force you to do. I like that the proto-repl library is a nice to have not a must have. Futhermore, I wouldn't like proto-repl to dictate how are my stacktraces displayed. There are tons of libraries out there which already do that and each one have their reasons (and users) to do it that way. Why should we tell the users, "this is the best and only way to get your stacktraces" ?
• On the technical side though, I am not even sure if it could work given the conditions necessary for the highlighting and hyperlinking to work. They both need to have a full-path to work, which means that even if you return a stacktrace data structure, you would still need to figure out the full-path where each and every function resides as that information is usually not included in the stacktrace; most of the filepath information is provided relative to the project classpath. That's the reason why I have to send some commands to the nrepl to get more information on the stacktrace (see here). Furthermore, we can't guess the filepath based on the namespace as in Clojure they don't need to match, usually they do, but it is not a requirement.

IMHO, I think the best way to proceed is to:

• check if the executed code returned an exception
• send a (repl/pst) command to the nrepl to get the stacktrace (as it is overwritten, it should work without a problem)
• parse it with regexes and send more commands to the nrepl requesting the missing information
• hyperlink and highlight the relevant pieces

[jasongilman]

RE feature going out this way: I'm ok with that. I really want it to be automatic but if I care enough I can implement it. My one caveat is that I'd like the pst stacktrace command to still also go to the REPL. Is that possible? I like seeing the stack traces there so they can be copy and pasted or viewed there.

RE "This is where we disagree." Proto REPL works much better with the proto repl library. You get completions and the save and recall feature. Cider requires middleware so it's not completely uncommon to require some additional project level configuration. I agree with you though that it's nice that it works without the library. If you're not going to implement the automatic portion now that I described it's a moot point anyway. The code wouldn't necessarily need to be put in the library. It could be sent at runtime instead of predefined as a function.

Furthermore, I wouldn't like proto-repl to dictate how are my stacktraces displayed.

You've explained something that I didn't get before about your approach. I thought you were reliant on the way repl/pst displayed things but I missed that we could make the parsing dynamic. I had mentioned that in a previous comment about using that as an approach and I missed that's what you were doing.

Do you think the code is ready for merging now? If so I'll merge it in and then I'll try it for a few days before doing a Proto REPL release with this code.

[carocad]

@jasongilman I think we have been discussing over things that we both agree on but that we have said in different ways jeje :D

After the last message I realized that it shouldn't be difficult to implement the automatic inline-exception so I will first implement that. This PR is ready for merging as it is, but I would like to implement first this feature as described in my last message.

it would greatly help me if you could check some other stacktrace's libraries and implement their regexes to support this new feature. The stacktrace.coffee is the only thing that you would need to modify. I think it is well documented but let me know if you need some help. (see this to get started).

[carocad]

@jasongilman now we should be good to go :)

Please, by all means test this PR as much as possible before publishing it as I don't have much free time anymore since I started my new job.

For the record:

• those exceptions messages that were duplicated are now ignored
• if we detect an evaluation error we request the stacktrace and return that as msg.ex (i.e. we fake an exception). Thus we have automatic inline-exceptions for any stacktrace library that we would like to support :) !!
• repl/pst no longer has a special treatment, it will print the stacktrace to the repl as expected
• the (fake) returned exception is also send to the repl pane but as an ink-error not as stderr

TODO: allow exceptions to be copyable. I have already talked with the ink team about it but we haven't solved the problem yet. Once it is fixed, we should be able to copy the stacktrace content

[jasongilman]

Thanks for the update. I've been a bit busy with work lately but I'm going to start trying it out.

[jasongilman]

I started testing and found that running all the tests doesn't seem to be working or it's not producing the output. I'm using the proto repl demo project. I just get

Testing clojure.core.async.impl.exec.threadpool

And then no more output. There must be some problem with the output redirection. If I do this on master things are working. I haven't had a chance to look into it yet.

[carocad]

I tried it and indeed there is a bug there. However, I am not sure if the bug is in my code or in the node-nrepl-client.

The problem is that I 'unsubscribed' proto-repl from the messageStream here. My understanding was that ALL messages were collected and send back here.

That approach worked for every case that I have tested. Nevertheless, for some reason when you run all tests, the MessageStream from both functions above are very different. The message collection only gets a fraction of the total amount of messages that are send to stdout. The output that you see in the repl is precisely the messages that are sent back. So there problem is not in the output redirection but rather where the messages are read and which messages do we consider valid. I don't know the reason of this difference and since you created the function that reads from the 'MessageSequence' you probably have a better understanding of why it is like that.

I could change the code to behave like it did before. Nevertheless that would introduce a race condition between the MessageHandler and the ResultHandler since we modify a message here.

I guess the bug is in the node-nrepl-client since I would expect the 'send' command to receive all messages back but please let me know if you have more insights into this.

[carocad]

@mauricioszabo I just saw that you are doing a similar work on your plugin clojure-plus. Would you be interested in helping us with this feature? Most of the work is ready but we need more hands (brains) to check that the edge cases (see above) are handled correctly. I would be happy to assist you if you need my help :)

[mauricioszabo]

@carocad wow, nice! I'll look at it as soon as possible! Can we make a checklist to see which edge cases were already corrected, and to unify this information in a single place?

[jasongilman]

Thanks @mauricioszabo and @carocad!

[carocad]

@mauricioszabo this is the current workflow inside proto-repl to support this feature

• whenever we send code to the repl we scan the response for an eval-error indicating that something went wrong
• when an exception happens
  • send more code to the repl requesting the stacktrace in a string format from repl/pst
  • the returned result contains a valid msg.value object so we change the message to contain an msg.ex to fake an exception
  • pass the faked msg object to the original resultHandler of the code that caused the exception
• For displaying, when we detect an exception
  • check the returned stacktrace against regex expressions for each supported stacktrace library (currently only clojure.repl)
  • when we support the stacktrace library
    • extract the namespace/function-name part and request the meta-information from the repl (filename, line number)
    • create hyperlinks and highlight the obtained filename + line number combination

Everything else should work as expected. There is one caveat though and it is precisely what we have not been able to solve: In order for the exceptions to be displayed in the console without duplicates we deactivated the stream of messages from the clojure repl (see here). Instead we rely on a bulk update from the repl (usually) send once the function execution ends. So far this approach works with the sole exception of the case mentioned by @jasongilman above.

I also confirmed this behavior in my branch. For some reason, whenever the user requests a full test run from a project, we only get a fraction of the information sent to std-out. This information is present on the stream of messages though, but not on the bulk update that we receive afterwards :(

One last comment. We currently don't support highlighting/hyperlinks to any external library (clojure itself included) because when an exception happens we immediately highlight the error lines. Since third-party code is compressed into jar files, we dont have a filename/path combination to pass to atom to perform the highlight/hyperlink. We could decompress them at runtime but so far I tried to keep it simple to avoid even more problematic behavior.

I hope this helps, let me know if you have further questions.

[carocad]

closing this due to lack of activity