Try to handle CIDER mistakes more gracefully

[sanjayl]

Not ready yet, but thought I'd see what you guys thought before going any further

Before submitting a PR make sure the following things have been done:

• The commits are consistent with our contribution guidelines
• You've added tests (if possible) to cover your change(s)
• All tests are passing (make test)
• The new code is not generating bytecode or M-x checkdoc warnings
• You've updated the changelog (if adding/changing user-visible functionality)
• You've updated the readme (if adding/changing user-visible functionality)
• You've updated the refcard (if you made changes to the commands listed there)

Thanks!

Unless specifically coded into the sync request, errors result in
stacktraces being thrown onto the REPL and focus changing to the REPL
buffer. These changes just log the exception info into a buffer and
notify the user of this in the message area. Easiest way to see how this
works is to select a region with unbalanced parens and then call a
cider-format-region-edn.

[Malabarba]

Don't set this variable. Let-bind inhibit-read-only to t instead.

[Malabarba]

Thanks. 👍

Could you show an example of what this would look like? And what's the current behaviour?

Also, the function names need to be namespaced.

[sanjayl]

Yeah sure, here's a screencast of the before:
https://youtu.be/lRfk5ic6euY
and afterwards with the PR:
https://youtu.be/X3zrGdlmHhE

Any preference on where the error management should live? I just put them in the nrepl-client code because that was proximal to where the errors were coming into the system

[bbatsov]

Note that the real problem in a way is this one - #1420

Ideally people would get stacktrace buffers when using sync nREPL requests and there won't be a need for another different error buffer. I'll go over your code a bit later.

[sanjayl]

Interesting...I'll try and make a version where everything goes through the standard stacktrace buffer.

I think that it'll be beneficial to ultimately have some kind of tiered error reporting system with varying degrees of interaction, attention, and logging. This would require doing some analysis of the stack traces at the middleware level and then have some standard hierarchy of error messages to use in the reply's status slot

[sanjayl]

Here's a demonstration of error reporting using a stacktrace buffer (the vanilla one, not the fancy one that relies on the stacktrace middleware). It was pretty much just a two line change in the existing code.
https://youtu.be/L4TS77nZMq8

Not a huge fan of it for this particular problem, but would be a nice option to have for sync request errors in general.

In order to get it to use the fancy stacktrace middleware, I was planning on keeping a cache of the caught java.lang.Exception's and stuffing a key to that cache into the reply packet we send back to the CIDER side. That way the CIDER side can request a StackTrace analysis after the fact without needing *e to be bound and it wouldn't matter if *e got changed out from underneath us or anything like that. (The error catching refactorings/abstractions that we've been discussing would be pretty useful for this to hide all this complexity)

[bbatsov]

Here's a demonstration of error reporting using a stacktrace buffer (the vanilla one, not the fancy one that relies on the stacktrace middleware). It was pretty much just a two line change in the existing code.
https://youtu.be/L4TS77nZMq8

This video is private. :-)

[sanjayl]

oops, my bad, give it a try again:
https://www.youtube.com/watch?v=L4TS77nZMq8

On Thu, Mar 17, 2016 at 1:56 AM, Bozhidar Batsov [email protected]
wrote:

Here's a demonstration of error reporting using a stacktrace buffer (the
vanilla one, not the fancy one that relies on the stacktrace middleware).
It was pretty much just a two line change in the existing code.
https://youtu.be/L4TS77nZMq8

This video is private. :-)

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1617 (comment)

[johngit22]

Thanks!
On Mar 17, 2016 2:14 AM, "sanjayl" [email protected] wrote:

oops, my bad, give it a try again:
https://www.youtube.com/watch?v=L4TS77nZMq8

On Thu, Mar 17, 2016 at 1:56 AM, Bozhidar Batsov <[email protected]

wrote:

Here's a demonstration of error reporting using a stacktrace buffer (the
vanilla one, not the fancy one that relies on the stacktrace middleware).
It was pretty much just a two line change in the existing code.
https://youtu.be/L4TS77nZMq8

This video is private. :-)

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
<#1617 (comment)

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1617 (comment)

[bbatsov]

Looks ok.

Not a huge fan of it for this particular problem, but would be a nice option to have for sync request errors in general.

Why not? I'd rather use something generic instead of having some special buffer type for middleware errors. I see the point of your initial solution, but it seems to me that's it's a bit excessive (and I don't like that the error buffer combines different data, that while useful debugging is likely going to confuse most users). I do agree, however, that it makes sense to display some message in the minibuffer saying that an "internal cider error occurred" or something like this in such situations, as obviously just dumping/showing a stacktrace is not terribly informative either.

In order to get it to use the fancy stacktrace middleware, I was planning on keeping a cache of the caught java.lang.Exception's and stuffing a key to that cache into the reply packet we send back to the CIDER side. That way the CIDER side can request a StackTrace analysis after the fact without needing *e to be bound and it wouldn't matter if *e got changed out from underneath us or anything like that. (The error catching refactorings/abstractions that we've been discussing would be pretty useful for this to hide all this complexity)

Yeah, I guess something like this might work.

[bbatsov]

Btw, you should check out https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html (or use M-x checkdoc).

[sanjayl]

Thanks for the pointers and documentation resources on the Elisp code! I'll definitely update with better organized and documented code once I have a more concrete idea of what to implement and what parts of the system it will touch. Ideally I'd like to pull all of the error handling related code out into a different namespace but I'm worried to make drastic changes since I don't understand Elisp or the CIDER side of things well at all, so I'll probably just start small.

Why not? I'd rather use something generic instead of having some special buffer type for middleware errors. I see the point of your initial solution, but it seems to me that's it's a bit excessive (and I don't like that the error buffer combines different data, that while useful debugging is likely going to confuse most users). I do agree, however, that it makes sense to display some message in the minibuffer saying that an "internal cider error occurred" or something like this in such situations, as obviously just dumping/showing a stacktrace is not terribly informative either.

Yeah, that's a good point...I don't have any compelling arguments for my case to be honest and the examples I put together are more just for brainstorming rather than a final destination. The next prototype that I'm planning will let you coarsely tune the error response, so maybe we'll be able to figure out what the best direction would be by getting some real world usage under our belts.

I will say that in light of the bug report in CIDER-nREPL that prompted this branch, it would be nice if bug reports would contain the request packet, reply packet, AND the stacktrace instead of just the stacktrace so that it would be a bit easier to replicate the problem. You're right that it is confusing though, so I'll have to think more about how to balance UI with QA needs.

Thanks again for the input!

[sanjayl]

Here's the demo of a sync-request error displayed in the nice stacktrace-op dependent buffer as you mentioned:

https://youtu.be/RsYtKBZniVs

I just added:

1. A tiny util ns to basically mock out a cache
2. Modified the u/err-info function so that it automatically registers every exception into the cache and stuffs a key into the response packet.
3. An op in the stacktrace middleware to look up an Exception from the cache instead of using *e

From there, I basically just used the same internals that we use on the CIDER side for handling the eval-type errors. Actually, there's very little that's new in these changes at all, just a couple lines here and there that are different.

I've pushed the CIDER-nREPL code to my repo under the store-stacktrace branch. It's very very (embarrassingly) rudimentary, so I didn't want to send over a PR, but let me know if it's going in the right direction and I'll try to polish it up and get it out soon.

Thanks!

[sanjayl]

Actually, now that I think about it, we don't even need the separate Stacktrace op, could just pass in an optional storage-key...If no storage key passed in, return *e. If key passed in, return stored stacktrace. If we did it that way, there'd be even fewer overall changes

[bbatsov]

Here's the demo of a sync-request error displayed in the nice stacktrace-op dependent buffer as you mentioned:

https://youtu.be/RsYtKBZniVs

Looks good!

Actually, now that I think about it, we don't even need the separate Stacktrace op, could just pass in an optional storage-key...If no storage key passed in, return *e. If key passed in, return stored stacktrace. If we did it that way, there'd be even fewer overall changes

Sounds like a good plan to me.

I will say that in light of the bug report in CIDER-nREPL that prompted this branch, it would be nice if bug reports would contain the request packet, reply packet, AND the stacktrace instead of just the stacktrace so that it would be a bit easier to replicate the problem. You're right that it is confusing though, so I'll have to think more about how to balance UI with QA needs.

Indeed. Finding the right balance is always hard. On a related note - the UI of the messages log itself should eventually be improved - e.g. it might be nice to be able to filter out certain message types, highlight requests and responses, font-lock errors differently, and fix the alignment of dictionaries... If only we had time. :-)

[sanjayl]

Submitting a different pull request which doesn't require the whole "storage" mess. This PR can be rejected.