Uncaught BrokenPipeError makes Neovim+deoplete unusable while error message is printing

[daveyarwood]

I run into this problem fairly often, where I am developing in Neovim, connected to a REPL, and for one reason or another, I end up needing to restart the REPL, so I Ctrl-C it and restart. This breaks the plugin's connection to the REPL, so the next time deoplete needs to autocomplete something (i.e. I start typing), my typing is interrupted by a stacktrace, which I have to ENTER through one line at a time:

[deoplete] Traceback (most recent call last):
[deoplete]   File "/home/dave/.vim/bundle/deoplete.nvim/rplugin/python3/deoplete/deoplete.py", line 123, in gather_results
[deoplete]     ctx['candidates'] = source.gather_candidates(ctx)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/async_clj.py", line 24, in gather_candidates
[deoplete]     return self.__cider_completion_manager.gather_candidates(context["complete_str"])
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/../../../../pythonx/async_clj_omni/fireplace.py", line 87, in gather_candidates
[deoplete]     ns)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/../../../../pythonx/async_clj_omni/cider.py", line 60, in cider_gather
[deoplete]     "ns": ns
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/../../../../pythonx/async_clj_omni/fireplace.py", line 53, in send
[deoplete]     self.wc.send(msg)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/vim_nrepl_python_client/nrepl/__init__.py", line 78, in send
[deoplete]     self._IO.write(message)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/vim_nrepl_python_client/nrepl/bencode.py", line 175, in write
[deoplete]     return _write_datum(v, self._file)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/vim_nrepl_python_client/nrepl/bencode.py", line 125, in _write_datum
[deoplete]     _write_datum(v, out)
[deoplete]   File "/home/dave/.vim/bundle/async-clj-omni/rplugin/python3/deoplete/sources/vim_nrepl_python_client/nrepl/bencode.py", line 127, in _write_datum
[deoplete]     out.flush()
[deoplete]   File "/usr/lib/python3.5/socket.py", line 593, in write
[deoplete]     return self._sock.send(b)
[deoplete] Could not get completions from: async_clj.  Use :messages for error details.

EDIT: I could have swore there was a BrokenPipeError printed at the bottom, before... I may have accidentally deleted that line. Ugh, sorry...

It's not clear to me if the root of the problem lies in deoplete or clj-async-omni, as I'm not sure how they interact with each other.

Assuming that clj-async-omni is at the top of the stack, would it make sense for clj-async-omni to catch BrokenPipeError and do something less cumbersome? (e.g. print an "nREPL connection was lost." message and stop trying to connect to it)

[SevereOverfl0w]

I feel like I used to catch this… I did extensive testing around it. It disappeared amongst my attempt to refactor the code. The best commit to view how this should look is https://github.com/clojure-vim/async-clj-omni/commit/5fd58b33cbf7bc674dccea645bd0a069f75583fe

No need to print anything in case of BrokenPipeError, what is important is to remove the connection from the list of active ones.

[daveyarwood]

I see -- so if the connection is lost, the idea is to forget about that connection and silently wait for a new one?

[SevereOverfl0w]

Removing the connection is to handle the case where someone is listening on a specific port (e.g. 5600) and then restarts the repl on that same port.

If you didn't pop the connection, 99% of users would never notice. As it would appear to immediately resume working using the new port as supplied by acid/fireplace.

[SevereOverfl0w]

This seems to be more complex by nature of me adopting an OO-style. My instinct tells me me to pass around a "deregister me" function into the right places so that it can be deregistered from the innermost point.

No idea if this is possible or nice in python though.

[daveyarwood]

I took a brief look at the code, and if I'm not mistaken, don't we need to re-add the try/catch around these lines?

Is it an issue of not having access to the list of connections in the context manager? If so, maybe a possible solution would be to pass the context manager around to functions that need it?

I would also argue that global state is not always bad. I feel like for some things like active connections, it can be useful to define them at the top level so that they're easily accessible when some function deep down in the innards needs to access them.

[SevereOverfl0w]

I think that global state is actually more confusing here.

The nrepl variable is expected to fulfil an "interface" (duck typed), this allows for us to support both acid and fireplace.

It's possible someday that a plugin would handle its own persistent connections (acid might do this, just coincidentally it uses the same python library that clj omni uses, so the api is identical).

On 12 Sep 2017 9:32 pm, "Dave Yarwood" notifications@github.com wrote:

I took a brief look at the code, and if I'm not mistaken, don't we need to re-add the try/catch around these lines https://github.com/clojure-vim/async-clj-omni/blob/master/pythonx/async_clj_omni/cider.py#L53-L61 ?

Is it an issue of not having access to the list of connections in the context manager? If so, maybe a possible solution would be to pass the context manager around to functions that need it?

I would also argue that global state is not always bad. I feel like for some things like active connections, it can be useful to define them at the top level so that they're easily accessible when some function deep down in the innards needs to access them.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/clojure-vim/async-clj-omni/issues/14#issuecomment-328975773, or mute the thread https://github.com/notifications/unsubscribe-auth/AF2h2lQpo4gHvOpLMqtmT72Ub0TRO-xnks5shupNgaJpZM4PU5Wb .

[SevereOverfl0w]

I didn't clarify, by using a global var, you suddenly need to be more aware about which client you're talking to, to choose the right global var for example.

On 12 Sep 2017 9:51 pm, "Dominic Monroe" monroef4@googlemail.com wrote:

I think that global state is actually more confusing here.

The nrepl variable is expected to fulfil an "interface" (duck typed), this allows for us to support both acid and fireplace.

It's possible someday that a plugin would handle its own persistent connections (acid might do this, just coincidentally it uses the same python library that clj omni uses, so the api is identical).

On 12 Sep 2017 9:32 pm, "Dave Yarwood" notifications@github.com wrote:

I took a brief look at the code, and if I'm not mistaken, don't we need to re-add the try/catch around these lines https://github.com/clojure-vim/async-clj-omni/blob/master/pythonx/async_clj_omni/cider.py#L53-L61 ?

Is it an issue of not having access to the list of connections in the context manager? If so, maybe a possible solution would be to pass the context manager around to functions that need it?

I would also argue that global state is not always bad. I feel like for some things like active connections, it can be useful to define them at the top level so that they're easily accessible when some function deep down in the innards needs to access them.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/clojure-vim/async-clj-omni/issues/14#issuecomment-328975773, or mute the thread https://github.com/notifications/unsubscribe-auth/AF2h2lQpo4gHvOpLMqtmT72Ub0TRO-xnks5shupNgaJpZM4PU5Wb .

[daveyarwood]

Ah, I see.

After looking at this a bit more, I think another solution might be to add an on_error callback argument to cider_gather. Then you could add a try/catch around the nrepl.send(...) part, calling the on_error callback in the event of a BrokenPipeError.

Then, in CiderCompletionManager.gather_candidates, the connection manager is in scope, so you could pass in an on_error callback that deregisters the connection.

If that approach sounds OK to you, I wouldn't make taking a crack at a PR for this.

[daveyarwood]

I tried hacking something up in my local clone of async-clj-omni in ~/.vim/bundle, and I think it might fix this issue. The Python I wrote is syntactically valid (initially it wasn't, and the plugin complained, then I fixed it), but I'm not 100% sure it will work yet. I should know soon, as I start using the plugin more today. If it works, I'm happy to make a PR out of my changes.

[SevereOverfl0w]

Look forward to it. Make sure to update for both fireplace & acid. No need to test the one you don't use, best effort is enough. Can always fix it.

[daveyarwood]

Hmm, it looks like the AcidManager class has a self.__conns, but it isn't being used at all. For now, should I preserve the existing behavior where BrokenPipeError is still thrown?

It seems like the ideal thing might be to abstract out the connection management stuff so that both fireplace.py and acid.py can use it, and have both implementations handle BrokenPipeError by removing the connection from self.__conns. But, I'm not sure I can expend that level of effort at the moment, especially given that I can't test the acid implementation.