Skip to content
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.

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

Abstract away op error handling #327

Merged
merged 1 commit into from Apr 1, 2016

Conversation

@sanjayl
Copy link
Contributor

sanjayl commented Mar 31, 2016

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

  • [x ] The commits are consistent with our contribution guidelines
  • [ x] You've added tests to cover your change(s)
  • [ x] All tests are passing
  • [ x] The new code is not generating reflection warnings
  • You've updated the readme (if adding/changing middleware)

Thanks!

A new util ns in cider.nrepl.middleware.util.error-handling which
provides the with-safe-transport macro, abstracting away transport,
error handling, and stacktrace analysis. Most middlewares were updated
to use this system, and the tests were updated to reflect these changes.

In order to utilize the nicely formatted stacktraces stuffed into these
responses, concurrent changes have been made to CIDER and will be
submitted as a PR as well.

sanjayl added a commit to sanjayl/cider that referenced this pull request Mar 31, 2016
This lets sync ops use the stacktrace viewer, just like how async ops
currently do. Requires some changes to the CIDER-nREPL code as well
which were submitted as CIDER-nREPL PR 327
clojure-emacs/cider-nrepl#327.
@sanjayl sanjayl referenced this pull request Mar 31, 2016
0 of 7 tasks complete
each type is discussed above in the `selector` method."
[pass-through & pairings]
`(fn [{op# :op transport# :transport :as msg#}]
(try

This comment has been minimized.

Copy link
@Malabarba

Malabarba Mar 31, 2016

Member

While I understand the intent behind error-handling the error-handler itself, this will cause a stack of dozens of nested try blocks. Maybe that's ok. But then, maybe we don't need this outer try block either.

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

Oh wow I didn't even think about how ugly this would look once macroexpanded. The outer try is needed so you can do the inline responses safely (say for something that has a constant reply), but we'll probably never use this and could remove it entirely.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

Yeah, let's do this.

@Malabarba

This comment has been minimized.

Copy link
Member

Malabarba commented Mar 31, 2016

Thanks, I like this implementation.
Looks like, you've done a lot more than error handling. 😄 From what I understand, you've written an error handler and an op-handler.

The op-handler (which I like) does indeed need that with-safe-transport macro.
But the error-handler (the exception catching code) could have been done in a middleware, outside that macro. In fact, I think I would still prefer if the exception handling was done by a middleware, this way it would also benefit some our ops that don't yet take advantage of the with-safe-transport macro, and it could even benefit middlewares from other plugins (like refactor-nrepl).
Here's what the middleware would look like:

(defn wrap-errors [h]
  (fn [msg]
    (try
      (h msg)
      (catch Exception e
        (transport/send (:transport msg)
                        (error-handler :default msg e))))))

(set-descriptor! #'wrap-errors {})

The only thing we lose from this implementation is the ability to specify a different error status (currently I see :toggle-trace-error and :macroexpansion-error are using this). However, I believe these custom error statuses were only introduced because we lacked proper error-handling in the middlewares. So now that we have it we don't need them anymore.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 31, 2016

I agree with @Malabarba about the middleware for pretty much the same reasons he outlined. This would also protect us from internal errors in nREPL itself.

The only thing we lose from this implementation is the ability to specify a different error status (currently I see :toggle-trace-error and :macroexpansion-error are using this). However, I believe these custom error statuses were only introduced because we lacked proper error-handling in the middlewares. So now that we have it we don't need them anymore.

What exactly is different there? The code looks exactly the same as in other middleware to me.


(defn pop-reply [msg]
(try (success msg (swap-inspector! msg inspect/up))
(catch Exception e (failure msg e :inspect-pop-error))))

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

I wonder if the Elisp code cares about those different error codes. Seems to me there's still room to cleanup/simplify all those ops.

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

You're right, the Elisp code has absolutely no idea about these error codes. They could be removed entirely, but I think they're good to have so that we can "tune" the error reporting UI based on the type of error returned (which is something I've been experimenting with). Eval/big errors should certainly get the stacktrace viewer, some other types should just get a minibuffer message, and other types of errors might just get swallowed. The user would also be able to customize this by fooling around with some variables.


(defn set-page-size-reply [msg]
(try (success msg (swap-inspector! msg inspect/set-page-size (:page-size msg)))
(catch Exception e (failure msg e :inspect-set-page-size-error))))

(defn wrap-inspect

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

Was this not updated because of the eval in it?

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

It's more because of how the inspect-reply function handles creating a response. It writes a non-terminated response and expects the {:status #{:done}} cap to happen deeper down in the handler. The macro expects a response that can be immediately terminated. I didn't feel like mucking around with this too much because I didn't understand it well.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

I see. Probably we should refactor this down the road. It's a pretty old middleware, that hasn't be updated significantly in quite a while.

@sanjayl

This comment has been minimized.

Copy link
Contributor Author

sanjayl commented Mar 31, 2016

Thanks, I like this implementation.
Looks like, you've done a lot more than error handling. 😄 From what I understand, you've written an error handler and an op-handler.

Thanks Artur...yeah, I might have taken some liberties with mission creep as I went through the code base. Probably time to figure out how to selectively commit hunks in magit 😆.

In fact, I think I would still prefer if the exception handling was done by a middleware, this way it would also benefit some our ops that don't yet take advantage of the with-safe-transport macro, and it could even benefit middlewares from other plugins (like refactor-nrepl).

I agree with @Malabarba about the middleware for pretty much the same reasons he outlined. This would also protect us from internal errors in nREPL itself.

These are really good points. A couple questions regarding how to implement this: Is there a way to ensure that the error-middleware is the outermost middleware, or will it have to be part of convention that the error-middleware gets preferential ordering in the stack? What about when we're dynamically injecting middleware (such as when refactor-nrepl gets injected on jack-in), can we guarantee that will be injected within the error-middleware?

What exactly is different there? The code looks exactly the same as in other middleware to me.

RIght now it looks the same because virtually none of the middleware is doing anything unique with regards to error handling, but having the error handlers defined at the level of the "op -> action" mapping affords a simple mechanism for op-specific custom error handling. This would still be possible but would be a bit more cumbersome if all exception handling was done within a catch-all middleware (ie, some lookup table that the error-middleware could use to resolve a custom error handler given the information in the msg and Exception).

In my mind, there should be a 3 tiered system:

  1. Middleware "in the know" would be able to deal with ops and their errors all in the same place for the vast majority of operations by using the with-safe-transport macro.
  2. The base-error-response function allows for standardized error handling for those small number of cases where the default macro isn't appropriate. (much like how util.misc/err-info is currently used...which is incidentally what refactor-nrepl uses for its error handling and why I couldn't remove that function entirely)
  3. The middleware would serve as a catch all and deal with cases where error-handling wasn't considered or is out of our control

Thoughts on this layered approach? Certainly would be simpler to only use a middleware, but I don't have a great feeling about dealing with ops and their errors in 2 very different places.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 31, 2016

RIght now it looks the same because virtually none of the middleware is doing anything unique with regards to error handling, but having the error handlers defined at the level of the "op -> action" mapping affords a simple mechanism for op-specific custom error handling.

That's a good point as well. There's something else to consider, though - you can have several different error codes per one op if several different things can go wrong. In such cases (not sure if we have them in the current codebase or not) you'd still have to roll out some custom error handling code.

Anyways, I guess I'd be fine with both approaches - it's obviously that they are both superior to what we're doing right now...

project.clj Outdated
@@ -56,7 +56,8 @@
:dependencies [[org.clojure/clojure "1.8.0-master-SNAPSHOT"]]}

:test-clj {:test-paths ["test/clj"]
:java-source-paths ["test/java"]}
:java-source-paths ["test/java"]
:resource-paths ["test/resources"]}

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

Why is this needed? I don't see anything related in the PR (guess I might be missing something).

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

Sorry, not related to the PR. When I was going through the code base, I saw that resouce-test wasn't actually testing anything -- it was in effect doing something like asserting (= nil nil) -- because the test.txt resource didn't have its enclosing directory added to the resource path.

This comment has been minimized.

Copy link
@bbatsov

bbatsov Mar 31, 2016

Member

I see. Guess this should be fixed in a separate PR.

This comment has been minimized.

Copy link
@sanjayl

sanjayl Mar 31, 2016

Author Contributor

OK, just submitted a PR for this, will rebase this branch off master if it gets merged in

@sanjayl

This comment has been minimized.

Copy link
Contributor Author

sanjayl commented Mar 31, 2016

That's a good point as well. There's something else to consider, though - you can have several different error codes per one op if several different things can go wrong. In such cases (not sure if we have them in the current codebase or not) you'd still have to roll out some custom error handling code.

Hmmm, interesting. Do you mean that a single op would travel down the middleware stack and throw errors in multiple middlewares?

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 31, 2016

Do you mean that a single op would travel down the middleware stack and throw errors in multiple middlewares?

I meant one op could potentially throw different errors in different situations. Right now we have just one error per op, but that mostly because we don't really process the errors on the client-side. E.g. there can be 5 different macroexpansion errors. Not a big deal right, just some food for thought.

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Mar 31, 2016

Guess you'll have to do that rebase. :-)

A new util ns in `cider.nrepl.middleware.util.error-handling` which
provides the `with-safe-transport` macro, abstracting away transport,
error handling, and stacktrace analysis. Most middlewares were updated
to use this system, and the tests were updated to reflect these changes.

In order to utilize the nicely formatted stacktraces stuffed into these
responses, concurrent changes have been made to CIDER and will be
submitted as a PR as well.
@sanjayl sanjayl force-pushed the sanjayl:abstracted-error-handling branch from 40d9aa5 to 9a81e27 Mar 31, 2016
@sanjayl

This comment has been minimized.

Copy link
Contributor Author

sanjayl commented Mar 31, 2016

Done! Took out the "outer try" from the macro and rebased after the resource-test fix

@Malabarba

This comment has been minimized.

Copy link
Member

Malabarba commented Apr 1, 2016

These are really good points. A couple questions regarding how to implement this: Is there a way to ensure that the error-middleware is the outermost middleware, or will it have to be part of convention that the error-middleware gets preferential ordering in the stack?

I don't know. I know that we could add :require #{"wrap-errors"} to every one of our middlewares. But maybe there's a simpler way to specify that wrap-errors should be above every other middleware.
@arrdem Would you know of a way to do that?

What about when we're dynamically injecting middleware, can we guarantee that will be injected within the error-middleware?

I think Cider's dynamic injection should not interfere with this in any way.

@Malabarba

This comment has been minimized.

Copy link
Member

Malabarba commented Apr 1, 2016

Oops, long night. I meant to ping @cemerick
Do you know if there's a way to ensure that a midddleware gets placed above all others in nREPL?

@bbatsov bbatsov merged commit 0fa99e5 into clojure-emacs:master Apr 1, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented Apr 1, 2016

I've decided to merge the PR in its current form. We can always do the middleware change later if we decide that'd be best. Thanks for the huge amount of your you did here @sanjayl! It's much appreciated!

@bbatsov

This comment has been minimized.

Copy link
Member

bbatsov commented on src/cider/nrepl/middleware/apropos.clj in 9a81e27 Apr 1, 2016

I noticed this a bit too late - this error was handled specially by the client code.

This comment has been minimized.

Copy link
Member

bbatsov replied Apr 1, 2016

An example of the situation I mentioned - that one op could return different error codes.

@sanjayl

This comment has been minimized.

Copy link
Contributor Author

sanjayl commented Apr 1, 2016

No problem! I'll be taking a stab at #313, the first part ("mute all middleware errors") definitely sounds straightforward. The "mute this specific middleware error" button sounds like a very interesting UI idea and I'll let you know if it's doable.

sanjayl added a commit to sanjayl/cider that referenced this pull request Apr 1, 2016
This lets sync ops use the stacktrace viewer, just like how async ops
currently do. Requires some changes to the CIDER-nREPL code as well
which were submitted as CIDER-nREPL PR 327
clojure-emacs/cider-nrepl#327.
sanjayl added a commit to sanjayl/cider that referenced this pull request Apr 1, 2016
This lets sync ops use the stacktrace viewer, just like how async ops
currently do. Requires some changes to the CIDER-nREPL code as well
which were submitted as CIDER-nREPL PR 327
clojure-emacs/cider-nrepl#327.
@sanjayl sanjayl deleted the sanjayl:abstracted-error-handling branch Apr 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.