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.

Sign up for GitHub

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

Jump to bottom

Avoid cider--error-phase-of-last-exception recursive loop #3607

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Avoid cider--error-phase-of-last-exception recursive loop #3607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6b53a5
Avoid `cider--error-phase-of-last-exception` recursive loop
vemv Jan 17, 2024
18cbc53
Add comments
vemv Jan 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## master (unreleased)

### Bugs fixed

- [#3605](https://github.com/clojure-emacs/cider/issues/3605): avoid `cider--error-phase-of-last-exception` recursive loop.

## 1.13.0 (2024-01-14)

### Changes
Expand Down
52 changes: 38 additions & 14 deletions cider-eval.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,7 +764,8 @@ and ON-SUCCESS-CALLBACK an optional callback.
The handler simply inserts the result value in BUFFER."
(let ((eval-buffer (current-buffer))
(res "")
(failed nil))
(failed nil)
(error-phase-requested nil)) ;; avoid requesting the phase more than once - can happen if there are errors during the phase nrepl sync request.
(nrepl-make-response-handler (or buffer eval-buffer)
;; value handler:
(lambda (_buffer value)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this if is duplicated a few times, so it might be good to make it some utility function instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nil
(setq error-phase-requested t)
(cider--error-phase-of-last-exception buffer)))
(end (or (car-safe (cdr-safe bounds)) bounds))
(end (when end
(copy-marker end))))
Expand Down Expand Up @@ -845,12 +849,16 @@ REPL buffer. This is controlled via
(when-let ((conn (with-current-buffer buffer
(cider-current-repl))))
(when (cider-nrepl-op-supported-p "analyze-last-stacktrace" conn)
(when-let* ((result (nrepl-send-sync-request (thread-last (map-merge 'list
'(("op" "analyze-last-stacktrace"))
(cider--nrepl-print-request-map fill-column))
(seq-mapcat #'identity))
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 4? That's a oddly specific number. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

(when-let* ((result (nrepl-send-sync-request (thread-last (map-merge 'list
'(("op" "analyze-last-stacktrace"))
(cider--nrepl-print-request-map fill-column))
(seq-mapcat #'identity))
conn
'abort-on-input ;; favor responsiveness over this feature, in case something went wrong.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I reproduced the issue, 'abort-on-input made it less terrible, so it seems wise to leave this here.

)))
(nrepl-dict-get result "phase")))))))

(defcustom cider-inline-error-message-function #'cider--shorten-error-message
"A function that will shorten a given error message,
Expand Down Expand Up @@ -905,17 +913,24 @@ when `cider-auto-inspect-after-eval' is non-nil."
(beg (when beg (copy-marker beg)))
(end (when end (copy-marker end)))
(fringed nil)
(res ""))
(res "")
(error-phase-requested nil)) ;; avoid requesting the phase more than once - can happen if there are errors during the phase nrepl sync request.
(nrepl-make-response-handler (or buffer eval-buffer)
;; value handler:
(lambda (_buffer value)
(setq res (concat res value))
(cider--display-interactive-eval-result res 'value end))
;; stdout handler:
(lambda (_buffer out)
(cider-emit-interactive-eval-output out))
;; stderr handler:
(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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

nil
(setq error-phase-requested t)
(cider--error-phase-of-last-exception buffer))))

(cider--maybe-display-error-as-overlay phase err end)

Expand All @@ -927,6 +942,7 @@ when `cider-auto-inspect-after-eval' is non-nil."
;; the error is 'right there' in the current line
;; and needs no jumping:
phase)))
;; done handler:
(lambda (buffer)
(if beg
(unless fringed
Expand All @@ -946,7 +962,8 @@ when `cider-auto-inspect-after-eval' is non-nil."
"Make a load file handler for BUFFER.
Optional argument DONE-HANDLER lambda will be run once load is complete."
(let ((eval-buffer (current-buffer))
(res ""))
(res "")
(error-phase-requested nil)) ;; avoid requesting the phase more than once - can happen if there are errors during the phase nrepl sync request.
(nrepl-make-response-handler (or buffer eval-buffer)
(lambda (buffer value)
(cider--display-interactive-eval-result value 'value)
Expand All @@ -963,7 +980,10 @@ Optional argument DONE-HANDLER lambda will be run once load is complete."
;; 1.- Jump to the error line:
(cider-handle-compilation-errors err eval-buffer)
(with-current-buffer eval-buffer
(let* ((phase (cider--error-phase-of-last-exception buffer))
(let* ((phase (if error-phase-requested
nil
(setq error-phase-requested t)
(cider--error-phase-of-last-exception buffer)))
;; 2.- Calculate the overlay position, which is the point (per the previous jump),
;; and then end-of-line (for ensuring the overlay will be rendered properly):
(end (save-excursion
Expand Down Expand Up @@ -1072,7 +1092,8 @@ and SOURCE-BUFFER the original buffer
This is used by pretty-printing commands."
;; NOTE: cider-eval-register behavior is not implemented here for performance reasons.
;; See https://github.com/clojure-emacs/cider/pull/3162
(let ((chosen-buffer (or buffer (current-buffer))))
(let ((chosen-buffer (or buffer (current-buffer)))
(error-phase-requested nil)) ;; avoid requesting the phase more than once - can happen if there are errors during the phase nrepl sync request.
(nrepl-make-response-handler
chosen-buffer
;; value handler:
Expand All @@ -1087,7 +1108,10 @@ This is used by pretty-printing commands."
(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
nil
(setq error-phase-requested t)
(cider--error-phase-of-last-exception buffer)))
(end (or (car-safe (cdr-safe bounds)) bounds))
(end (when end
(copy-marker end))))
Expand Down
Loading