fix a few hangs due to missing terminators

[raymond-w-ko]

Like many other people I keep getting unexplainable hangs, so I decided to do some debugging. It turns out that vim-fireplace expects a status: ["done"] inside the return message as a terminator, but there are last messages returned that don't have this.

So far I found 2:

1. If you break something during a :Require, like a wrong symbol, It tries to fetch a strack trace, which returns one without this terminator.
2. Randomly (?) it tries to return this packet as the last message:

   RESPONSE:
   {'changed-namespaces': {},
   'id': 'fireplace-hostname-1538883267-20',
   'repl-type': 'clj',
   'session': '2b7e5a1e-748b-40c9-b65e-9a9b2c7c49a2',
   }

I am using cider-nrepl 0.18.0 if that makes a difference.

I am not sure what is the "truly correct" way, but the fix seems simple and safe enough. The truly correct way seems to be rewrite vim-fireplace using a persistent connection and command stream, but that seems really hard / impossible to do...

This patch detects these message and places the missing terminator if there is not a status.

[devth]

@raymond-w-ko awesome! I've been seeing tons of these random hangups.

[raymond-w-ko]

Woops, bungled my last commit message. It meant to say that it returned a status of ['state']

[devth]

Not sure if related but I also see tons of messages like:

SEVERE: Unhandled REPL handler exception processing message {:id fireplace-LM-SJC-11008489-1539119155-7, :op close, :session 19667562-7257-4b93-b444-87607d2bb9ca}
java.net.SocketException: Socket closed
        at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:118)
        at java.net.SocketOutputStream.write(SocketOutputStream.java:155)
        at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:82)
        at java.io.BufferedOutputStream.flush(BufferedOutputStream.java:140)
        at clojure.tools.nrepl.transport$bencode$fn__20970.invoke(transport.clj:103)
        at clojure.tools.nrepl.transport.FnTransport.send(transport.clj:28)
        at clojure.tools.nrepl.middleware.session$close_session.invokeStatic(session.clj:150)
        at clojure.tools.nrepl.middleware.session$close_session.invoke(session.clj:146)
        at clojure.tools.nrepl.middleware.session$session$fn__21444.invoke(session.clj:189)

in my repl when trying to load code from vim using fireplace.

[raymond-w-ko]

I also get this message alot. Do you directly type stuff into the REPL often, or just :Require and cpp things? If so, it might be worth me attempting to figuring out what is triggering this error.

[devth]

Mostly from cpr and cpp

[tpope]

So the issue here is that nREPL ops conventionally end with a status message, but the protocol doesn't enforce this, and the CIDER folks, who don't rely on it, occasionally forget it. At least, that's definitely what's going on with the stacktrace endpoint.

I'll probably give up and merge this, but could someone do a bit of due diligence on the cider-nrepl end and see if they can come up with a fix on that end?

[raymond-w-ko]

I am annoyed to a point where I am attempting to create a new plugin based on this one (https://github.com/raymond-w-ko/vim-plasmaplace) that uses vim8 built-in terminal to host a lein repl, term_sendkeys() to send commands, and eterm escape code suported by the vim terminal to dispatch commands back. The biggest challenge is asynchronous callback, and I am not sure if things like omnicomplete will ever work. It will be quite a while before it anything comes out of this.

[bbatsov]

So the issue here is that nREPL ops conventionally end with a status message, but the protocol doesn't enforce this, and the CIDER folks, who don't rely on it, occasionally forget it. At least, that's definitely what's going on with the stacktrace endpoint.

It might be some oversight on our end, but as we maintain both nREPL and CIDER these days we're generally quite careful with following the protocol conventions. Keep in mind, however that complex responses are formed of multiple response messages and just the last one is supposed to have a status. I have a feeling that this might be mishandled in fireplace. What clients usually do is to track the related responses together based on their request id.

[tpope]

Now that I have a bit of time to dig in, I can't actually reproduce this. The stacktrace op does include the done status, and Fireplace handles it correctly. I don't suppose I'm lucky enough that it's already been fixed?

[tpope]

I finally got to the bottom of this. I was able to reproduce after using an invalid identifier as my error trigger rather than dividing by zero which is my usual goto. Python 3's socket.makefile() returns a buffered IO object by default, contrary to the documentation. The :status [:done] message was getting read into a buffer and discarded when the file was closed. The changed-namespaces thing was a red herring, as disabling Cider eliminated it without fixing the issue.