Avoid cider--error-phase-of-last-exception recursive loop
#3607
Conversation
| (lambda (buffer err) | ||
| (cider-emit-interactive-eval-err-output err) | ||
|
|
||
| (let ((phase (cider--error-phase-of-last-exception buffer))) | ||
| (let ((phase (if error-phase-requested |
There was a problem hiding this comment.
This is the main fix
I could only immediately reproduce it for cider-interactive-eval-handler. But I used the same pattern in the other handlers as it's easy enough, and harmless.
There was a problem hiding this comment.
You might want to add some comment explaining the need for the toggling between t/nil of this flag, as it's definitely not going to be obvious to most people.
| (cider--nrepl-print-request-map fill-column)) | ||
| (seq-mapcat #'identity)) | ||
| conn | ||
| 'abort-on-input ;; favor responsiveness over this feature, in case something went wrong. |
There was a problem hiding this comment.
When I reproduced the issue, 'abort-on-input made it less terrible, so it seems wise to leave this here.
| @@ -781,7 +782,10 @@ The handler simply inserts the result value in BUFFER." | |||
| (when (and source-buffer | |||
| (listp bounds)) ;; if it's a list, it represents bounds, otherwise it's a string (code) and we can't display the overlay | |||
| (with-current-buffer source-buffer | |||
| (let* ((phase (cider--error-phase-of-last-exception buffer)) | |||
| (let* ((phase (if error-phase-requested | |||
There was a problem hiding this comment.
It seems this if is duplicated a few times, so it might be good to make it some utility function instead.
There was a problem hiding this comment.
Not sure if an abstraction could capture the necessary pattern
At most I can imagine a macro that ensured that a local variable error-phase-requested exists.
| conn))) | ||
| (nrepl-dict-get result "phase")))))) | ||
| (let ((nrepl-err-handler (lambda (&rest _))) ;; ignore any errors during this request to avoid any recursion | ||
| (nrepl-sync-request-timeout 4)) ;; ensure that this feature cannot possibly create an overly laggy UX |
There was a problem hiding this comment.
Why 4? That's a oddly specific number. :-)
There was a problem hiding this comment.
4 - ample enough to give it time to be slow (especially if there was some sort of cache to be hit), tight enough to not negatively impact the UX
I'd be fine with 3 or 5 as well 😄
|
Apart from my small remarks the changes look reasonable to me. |
|
Thanks for the review! I was hesitant about an abstraction (even more so with #3606), but if you felt it was needed we could keep it in the tracker. |
|
No, it's not a big deal - I'm just always wary of repeated code, as usually we get to a situation where someone forgets to make some change at every location. Anyways, I doubt this code is going to see much change going forward. |
This was an extra measure included as part of #3607 It resulted in `*cider-error*` not being shown, in certain scenarios. That particular measure wasn't critical in avoiding the issue tackled in that PR.
This was an extra measure included as part of #3607 It resulted in `*cider-error*` not being shown, in certain scenarios. That particular measure wasn't critical in avoiding the issue tackled in that PR.
Fixes #3605
Cheers - V