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

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

merged 2 commits into from
Jan 17, 2024

Conversation

vemv
Copy link
Member

@vemv vemv commented Jan 17, 2024

Fixes #3605

Cheers - V

(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.

(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.

@vemv vemv requested a review from bbatsov January 17, 2024 07:34
@@ -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.

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 😄

@bbatsov
Copy link
Member

bbatsov commented Jan 17, 2024

Apart from my small remarks the changes look reasonable to me.

@vemv vemv merged commit f7f8e2c into master Jan 17, 2024
33 of 34 checks passed
@vemv vemv deleted the 3605 branch January 17, 2024 14:47
@vemv
Copy link
Member Author

vemv commented Jan 17, 2024

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.

@bbatsov
Copy link
Member

bbatsov commented Jan 17, 2024

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.

vemv added a commit that referenced this pull request Jan 21, 2024
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.
vemv added a commit that referenced this pull request Jan 21, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cider--error-phase-of-last-exception can call itself when the cider-nrepl middleware is faulty
2 participants