search

joaotavora / sly

Sylvester the Cat's Common Lisp IDE
1.27k stars 145 forks source link

Sly REPL becomes unresponsive after debugger is entered and thread killed #122

Closed defaultxr closed 5 years ago

defaultxr commented 7 years ago

After the debugger is entered, the Sly REPL appears to become unresponsive to any further attempts to evaluate forms. Using the latest Sly from MELPA, Emacs 25.2.1, SBCL 1.3.20.

  1. emacs -Q -L . -l sly-autoloads --eval '(setq inferior-lisp-program "sbcl")' -f sly
  2. In the Sly mREPL: (/ 1 0)
  3. In the debugger window: q
  4. In the Sly mREPL: (print 'hi)

Then it never returns; pressing any further keys in the REPL just gives a "[sly] REPL is busy" message.

Here is my *sly-events for sbcl* buffer after doing the above:

(:emacs-rex
 (slynk:connection-info)
 nil t 1)
(:return
 (:ok
  (:pid 20007 :style :spawn :encoding
    (:coding-systems
     ("utf-8-unix" "iso-latin-1-unix"))
    :lisp-implementation
    (:type "SBCL" :name "sbcl" :version "1.3.20" :program "/usr/bin/sbcl")
    :machine
    (:instance "hypermulti" :type "X86-64" :version "Intel(R) Core(TM) i7-6920HQ CPU @ 2.90GHz")
    :features
    (:slynk :quicklisp :sb-bsd-sockets-addrinfo :asdf-package-system :asdf3\.1 :asdf3 :asdf2 :asdf :os-unix :non-base-chars-exist-p :asdf-unicode :64-bit :64-bit-registers :alien-callbacks :ansi-cl :ash-right-vops :c-stack-is-control-stack :common-lisp :compact-instance-header :compare-and-swap-vops ...)
    :modules
    ("SLYNK-COMPLETION" "SB-CLTL2" "SB-INTROSPECT" "SB-BSD-SOCKETS" "SB-POSIX" "ASDF" "asdf" "UIOP" "uiop")
    :package
    (:name "COMMON-LISP-USER" :prompt "CL-USER")
    :version "1.0.0-beta-2"))
 1)
(:emacs-rex
 (slynk:slynk-add-load-paths
  '("/home/modula/.emacs.d/elpa/sly-20170413.557/contrib/"))
 nil t 2)
(:indentation-update
 (("with-alien" 1
   ("SB-BSD-SOCKETS-INTERNAL" "SB-POSIX" "SB-VM" "SB-UNIX" "SB-THREAD" "SB-REGALLOC" "SB-KERNEL" "SB-INT" "SB-IMPL" "SB-FASL" "SB-EXT" "SB-C" "SB-ALIEN" "COMMON-LISP-USER"))
  ("with-auxiliary-alien-types" 1
   ("SB-ALIEN"))
  ("alien-lambda" 2
   ("SB-ALIEN"))
  ("define-alien-callback" 3
   ("SB-ALIEN"))
  ("maybe-with-pinned-objects" 2
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("define-alien-type-translator" 2
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("without-scheduling" 1
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("define-instruction-macro" 2
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("assemble" 1
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("note-dependencies" 1
   ("SB-ASSEM"))
  ("with-modified-segment-index-and-posn" 1
   ("SB-ASSEM"))
  ("sc-case" 1
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("def-ir1-translator" 2
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("defknown" 4
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("with-compiler-error-resignalling" 0
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("defoptimizer" 2
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("define-vop" 1
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("define-move-fun" 3
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("deftransform" 2
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("define-source-transform" 2
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ...))
(:return
 (:ok nil)
 2)
(:emacs-rex
 (slynk:slynk-require
  '("slynk-indentation" "slynk-stickers" "slynk-trace-dialog" "slynk-package-fu" "slynk-fancy-inspector" "slynk-arglists" "slynk-mrepl"))
 nil t 3)
(:return
 (:ok
  (("SLYNK-MREPL" "SLYNK-ARGLISTS" "SLYNK-FANCY-INSPECTOR" "SLYNK-UTIL" "SLYNK-PACKAGE-FU" "SLYNK-TRACE-DIALOG" "SLYNK-STICKERS" "SLYNK-INDENTATION" "SLYNK-COMPLETION" "SB-CLTL2" "SB-INTROSPECT" "SB-BSD-SOCKETS" "SB-POSIX" "ASDF" "asdf" "UIOP" "uiop")
   ("slynk-mrepl" "slynk-arglists" "slynk-fancy-inspector" "slynk-package-fu" "slynk-trace-dialog" "slynk-stickers" "slynk-indentation")))
 3)
(:emacs-rex
 (slynk-mrepl:create-mrepl 1)
 nil t 4)
(:channel-send 1
           (:open-dedicated-output-stream 32911 nil))
(:channel-send 1
           (:prompt "COMMON-LISP-USER" "CL-USER" 0))
(:return
 (:ok
  (1 1))
 4)
(:indentation-update
 (("with-alien"
   (4 "&body")
   ("SB-BSD-SOCKETS-INTERNAL" "SB-POSIX" "SB-VM" "SB-UNIX" "SB-THREAD" "SB-REGALLOC" "SB-KERNEL" "SB-INT" "SB-IMPL" "SB-FASL" "SB-EXT" "SB-C" "SB-ALIEN" "COMMON-LISP-USER"))
  ("alien-size"
   (4
    ("&whole" 4))
   ("SB-BSD-SOCKETS-INTERNAL" "SB-POSIX" "SB-VM" "SB-UNIX" "SB-THREAD" "SB-REGALLOC" "SB-KERNEL" "SB-INT" "SB-IMPL" "SB-FASL" "SB-EXT" "SB-C" "SB-ALIEN" "COMMON-LISP-USER"))
  ("with-auxiliary-alien-types"
   (4 "&body")
   ("SB-ALIEN"))
  ("alien-lambda"
   (4 4 "&body")
   ("SB-ALIEN"))
  ("define-alien-callback"
   (4 4 4 "&body")
   ("SB-ALIEN"))
  ("maybe-with-pinned-objects"
   (4 4 "&body")
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("define-alien-type-method"
   (("&whole" 4)
    nil "&rest" nil)
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("define-alien-type-class"
   (("&whole" 4 1 "&rest" 1)
    "&rest" nil)
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("define-alien-type-translator"
   (4 4 "&body")
   ("SB-VM" "SB-REGALLOC" "SB-KERNEL" "SB-C" "SB-ALIEN-INTERNALS" "SB-ALIEN"))
  ("without-scheduling"
   (("&whole" 4
     ("&whole" 1 1 ...))
    "&body")
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("define-instruction-macro"
   (4 4 "&body")
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("emit-alignment"
   (4
    ("&whole" 4))
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("assemble"
   (("&whole" 4 1 1 "&rest" 1)
    "&body")
   ("SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C" "SB-ASSEM"))
  ("note-dependencies"
   (("&whole" 4)
    "&body")
   ("SB-ASSEM"))
  ("with-modified-segment-index-and-posn"
   (("&whole" 4)
    "&body")
   ("SB-ASSEM"))
  ("sc-case"
   (4 "&body")
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("def-ir1-translator"
   (4
    ("&whole" 4)
    "&body")
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("defknown"
   (4 4 4
      ("&whole" 4 1
       ("&whole" 1 1 ...))
      "&body")
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("with-compiler-error-resignalling"
   ("&body")
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ("defoptimizer"
   (4
    ("&whole" 4 1
     ("&whole" 1)
     "&rest" nil)
    "&body")
   ("SB-CLTL2" "SB-X86-64-ASM" "SB-VM" "SB-REGALLOC" "SB-FASL" "SB-C"))
  ...))
(:emacs-channel-send 1
             (:process "(/ 1 0)"))
(:channel-send 1
           (:prompt "COMMON-LISP-USER" "CL-USER" 1 "#<DIVISION-BY-ZERO {1001F050E3}>"))
(:debug 1 1
    ("arithmetic error DIVISION-BY-ZERO signalled\nOperation was (/ 1 0)." "   [Condition of type DIVISION-BY-ZERO]" nil)
    (("RETRY" "Retry SLY mREPL evaluation request.")
     ("*ABORT" "Return to SLY's top level.")
     ("ABORT" "abort thread (#<THREAD \"sly-channel-1-mrepl-remote-1\" RUNNING {1001C9AE73}>)"))
    ((0 "(SB-KERNEL::INTEGER-/-INTEGER 1 0)")
     (1 "(/ 1 0)")
     (2 "(SB-INT:SIMPLE-EVAL-IN-LEXENV (/ 1 0) #<NULL-LEXENV>)")
     (3 "(EVAL (/ 1 0))")
     (4 "((LAMBDA NIL :IN SLYNK-MREPL::MREPL-EVAL-1))"
        (:restartable t))
     (5 "(SLYNK::CALL-WITH-RETRY-RESTART \"Retry SLY mREPL evaluation request.\" #<CLOSURE (LAMBDA NIL :IN SLYNK-MREPL::MREPL-EVAL-1) {1001F0297B}>)"
        (:restartable t))
     (6 "((LAMBDA NIL :IN SLYNK-MREPL::MREPL-EVAL-1))"
        (:restartable t))
     (7 "((LAMBDA NIL :IN SLYNK::CALL-WITH-LISTENER))"
        (:restartable t))
     (8 "(SLYNK::CALL-WITH-BINDINGS ((*PACKAGE* . #<PACKAGE \"COMMON-LISP-USER\">) (*) (**) (***) (/) (//) ...) #<CLOSURE (LAMBDA NIL :IN SLYNK::CALL-WITH-LISTENER) {1001F025EB}>)"
        (:restartable t))
     (9 "(SLYNK-MREPL::MREPL-EVAL-1 #<SLYNK-MREPL::MREPL mrepl-1-1> \"(/ 1 0)\")"
        (:restartable t))
     (10 "(SLYNK-MREPL::MREPL-EVAL #<SLYNK-MREPL::MREPL mrepl-1-1> \"(/ 1 0)\")"
         (:restartable t))
     (11 "(SLYNK::PROCESS-REQUESTS NIL)"
         (:restartable t))
     (12 "((LAMBDA NIL :IN SLYNK::SPAWN-CHANNEL-THREAD))"
         (:restartable t))
     (13 "((LAMBDA NIL :IN SLYNK::SPAWN-CHANNEL-THREAD))"
         (:restartable t))
     (14 "(SLYNK-SBCL::CALL-WITH-BREAK-HOOK #<FUNCTION SLYNK:SLYNK-DEBUGGER-HOOK> #<CLOSURE (LAMBDA NIL :IN SLYNK::SPAWN-CHANNEL-THREAD) {1001CA000B}>)")
     (15 "((FLET SLYNK-BACKEND:CALL-WITH-DEBUGGER-HOOK :IN \"/home/modula/.emacs.d/elpa/sly-20170413.557/slynk/backend/sbcl.lisp\") #<FUNCTION SLYNK:SLYNK-DEBUGGER-HOOK> #<CLOSURE (LAMBDA NIL :IN SLYNK::SPAWN-CHA..")
     (16 "((LAMBDA NIL :IN SLYNK::CALL-WITH-LISTENER))"
         (:restartable t))
     (17 "(SLYNK::CALL-WITH-BINDINGS ((*PACKAGE* . #<PACKAGE \"COMMON-LISP-USER\">) (*) (**) (***) (/) (//) ...) #<CLOSURE (LAMBDA NIL :IN SLYNK::CALL-WITH-LISTENER) {1001CA004B}>)"
         (:restartable t))
     (18 "((LAMBDA NIL :IN SLYNK::SPAWN-CHANNEL-THREAD))"
         (:restartable t))
     (19 "((FLET \"WITHOUT-INTERRUPTS-BODY-4\" :IN SB-THREAD::INITIAL-THREAD-FUNCTION-TRAMPOLINE))"))
    nil)
(:debug-activate 1 1 nil)
(:emacs-rex
 (slynk:invoke-nth-restart-for-emacs 1 2)
 nil 1 5)
(:return
 (:abort "NIL")
 5)
(:debug-return 1 1 nil)
(:channel-send 1
           (:evaluation-aborted "#<DIVISION-BY-ZERO {1001F050E3}>"))
(:channel-send 1
           (:prompt "COMMON-LISP-USER" "CL-USER" 0))
(:emacs-channel-send 1
             (:process "(print 'hi)"))

If there is any more information I can provide, let me know.

joaotavora commented 7 years ago

Can't reproduce even with the nice reproduction recipe, sorry. Tried with Emacs 25.2 and SBCL 1.3.20.

Is this a source compiled sbcl or one of the stock binaries on https://sourceforge.net/projects/sbcl/files/sbcl/1.3.20/ ?

According to the log, the REPL is indeed busy since the lisp hasn't respoded to the :emacs-channel-send, but I wonder if its dead. Can you try M-x sly-mrepl-new, to see if you can open a different repl.

mfiano commented 7 years ago

I cannot reproduce this either using Emacs 25.2, latest sly master branch, and SBCL 1.3.20

joaotavora commented 7 years ago

Thanks for the double check @mfiano

defaultxr commented 7 years ago

Very strange, now I can't reproduce it either. It was happening consistently before even after restarting Emacs without init files. Sorry for wasting your time, I'll close this issue, and if it happens again I'll try your suggestion. Thanks.

defaultxr commented 7 years ago

OK, I think I see what the issue is. I was wrong, it doesn't happen when I press q in the debugger; it happens when I press 2, which is the [ABORT] abort thread (#<THREAD "sly-channel-3-mrepl-remote-3" RUNNING {1003494B83}>) restart. I don't think this happens in Slime, but I can definitely avoid doing that in Sly. And using M-x sly-mrepl-new does allow me to continue in the same session if I do end up using that restart accidentally. This seems like a bug to me so I'm reopening but feel free to close this if it's not.

joaotavora commented 7 years ago

Hmmm, aborting the thread that is running the REPL is a sure-fire way to block said REPL...

SLIME does have a way to restart the thread if it's killed, so that's why you don't see it. Perhaps SLY could have something similar...

defaultxr commented 7 years ago

That makes sense to me and in retrospect should've been obvious. So I guess technically NOT a bug, but more just poor expectations on my part due to my Slime habits.

hjudt commented 5 years ago

Yes it would. This is a duplicate of an existing issue (don't know which exactly). It's not super important because if you get into the habit of using a to select abort restarts you won't get into this situation. It's also reasonably easy to start a new REPL manually.

Not super important but it is really bad user experience. I can also press `q' and not stumble over this, but the reason alone that I had to figure out what I did "wrong" and report a bug about it (as others did before me) confirms this imho, and I have been a happy slime user for quite a while, and also a sly user for several months, so I'd say I've already got some acquaintance with these tools. It is usually a very bad idea to bite the hand that feeds you, so in 99% of all cases no one would want to kill that thread but rather choose to kill the REPL itself if something bad happens ;-) Among those options available, the consequences of choosing this special option might also not be obvious immediately and one wonders why the REPL suddenly stops working.

So I guess technically NOT a bug, but more just poor expectations on my part due to my Slime habits.

It may not be a bug, but it looks like one.

joaotavora commented 5 years ago

@hjudt ACK to all that, but this is all subjective. Anyway, It doesn't seem so hard to fix after all. It should at least teardown the REPL more gracefully (I could enhance the message to tell the user to M-x sly-mrepl-new to start a new one).

I'll see if the auto-restart thing is doable.

joaotavora commented 5 years ago

It may not be a bug, but it looks like one.

And this is why I marked it "bug".... (and "minor")

joaotavora commented 5 years ago

So @hjudt I think this is just about fixed, but needs testing (and your opinion). There are now two ways and two commits to fix this:

joaotavora commented 5 years ago

As I predicted, there are some issues with the auto-restart REPL thread. I think I've fixed some (all?) in the latest fe31948 commit...

hjudt commented 5 years ago

I have tested the 122 branch with the steps in issue #242 and this works fine. For me, this solution works best, as the user doesn't have to do anything and still gets notified that he would have shot himself in his foot. Perhaps the message could be improved to show the user a better way (like appending "Just use 'q'?" as a suggestion), but that is only a nit-pick.

joaotavora commented 5 years ago

I don't know... If we're going to restart the thread everytime (and only iff) the user chooses the restart we might as well not have the restart at all

hjudt commented 5 years ago

Probably. If the restart is useless after all (is it?), then maybe it would be easier to remove it and let the user handle disconnecting and reconnecting manually when necessary. Honestly, I am not sure why I choose this restart. Returning to the top level might make more sense, but maybe it is just because it usually appears at the bottom of the list and thus can be seen faster.

joaotavora commented 5 years ago

is it?

The only use I see for it is interrupting a lengthy process in a secondary REPL that you don't plan to use any more. It could be a shortcut to interrupting and tearing down in two separate steps. But that's as pretty thin use case.

Returning to the top level might make more sense,

It certainly does. That's what I use 9 times or of 10.

but maybe it is just because it usually appears at the bottom of the list and thus can be seen faster.

The list is usually 4 or 5 long for me, and I can pick or individual restarts easily. Plus I only use q and a for the last ones.

In summary i think hiding that restart would be best. But I don't think it's easy, as it's established by the implementation's thread library.

hjudt commented 5 years ago

I am still convinced the solution as you have presented it in branch 122 is the best one. The user will get notified that something didn't happen as it was supposed to (and maybe recognize it should be done differently) but will still have a working REPL and not wonder why it suddenly got stuck on the next input. Even if that doesn't change the user's behaviour, it should not cause any problems.

Fiddling around with an implementation's own restarts might not be a good idea, to me it seems more prone to breakage and I wonder whether changing that default behaviour would be worth the efforts. Also, this would probably have to be done for every implementation which does sound like a maintenance burden or might even be impossible or at least hard to accomplish.

joaotavora commented 5 years ago

@hjudt I commited the branch to master. Let's try it for a while, if something goes wrong I will go back to the more conservative commit (the one that tears down gracefully and tells you how to restart a REPL, you lose the history and backreferences)