Uncaught BrokenPipeError makes Neovim+deoplete unusable while error message is printing about async-clj-omni HOT 11 CLOSED

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 5fd58b3

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

from async-clj-omni.

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

from async-clj-omni.

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.

from async-clj-omni.

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.

from async-clj-omni.

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.

from async-clj-omni.

from async-clj-omni.

from async-clj-omni.

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.

from async-clj-omni.

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.

from async-clj-omni.

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.

from async-clj-omni.

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.

from async-clj-omni.