search

clojure-emacs / cider

The Clojure Interactive Development Environment that Rocks for Emacs
https://cider.mx
GNU General Public License v3.0
3.54k stars 646 forks source link

refresh ops shouldn't have a timeout #3652

Closed vemv closed 5 months ago

vemv commented 5 months ago 
nrepl-send-sync-request: Sync nREPL request timed out (op refresh-clear) after 10 secs

Messages such as these don't make much sense to me. Code reloading can take arbitrarily long.

Timing out can invite the user to try again while code reloading is already in progress (thankfully we have locking in place, but still leads to unnecessary queueing)

I'd say that for these ops in particular, we could have a timeout of about 3 minutes?

bbatsov commented 5 months ago

Very few projects would need a bigger timeout and the users can easily change it. I’m not sure there’s much we can improve here.

On Thu, May 2, 2024, at 7:00 PM, vemv wrote:

nrepl-send-sync-request: Sync nREPL request timed out (op refresh-clear) after 10 secs Messages such as these don't make much sense to me. Code reloading can take arbitrarily long.

Timing out can invite the user to try again while code reloading is already in progress (thankfully we have locking in place, but still leads to unnecessary queueing)

I'd say that for these ops in particular, we could have a timeout of about 3 minutes?

— Reply to this email directly, view it on GitHub https://github.com/clojure-emacs/cider/issues/3652, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAZLSQDMQVGCYYQHV5PV4LZAJWK7AVCNFSM6AAAAABHEC2S4SVHI2DSMVQWIX3LMV43ASLTON2WKOZSGI3TMMBSHEZTSMY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

vemv commented 5 months ago

I see things differently - many commercial-sized projects can easily need 30s+ for a full-blown refresh.

Also, users don't necessarily are familar with (or even will pay much attention to) "nrepl timeouts".

Test suite running is similarly unbounded?

vemv commented 5 months ago

A touch more of tech analysis:

A very cool alternative approach would be to remove the locking from https://github.com/clojure-emacs/cider-nrepl/blob/79611e47615bc00ea00fe9c265da896976324ae0/src/cider/nrepl/middleware/refresh.clj#L110 , refactoring it to merely enqueue a clearing. So next time refresh-reply is hit, it checks the queue, and performs a clearing if due.