search

nrepl / piggieback

nREPL support for ClojureScript REPLs
480 stars 48 forks source link

Ambly setup twice and no tear-down #54

Closed mfikes closed 9 years ago

mfikes commented 9 years ago

(This is not necessarily a piggieback issue, and could be Ambly, but logging it here for discussion.)

When I piggieback Ambly, I'm finding that Ambly's -setup initially fails (it can't make a WebDAV connection for some reason and throws from -setup), but when I hit <RETURN> after this, -setup is called again (with WebDAV succeeding), and things working properly and the REPL going live.

Then, when I type :cljs/quit, Ambly's -tear-down is evidently not called, leaving the WebDAV mount running.

mfikes commented 9 years ago

OK... I found that, since -tear-down is a no-op, the Ambly code that deals with "leftover" WebDAV mounts was evidently not robust and needed a small sleep to fix it.

So now the only real question I have is regarding -tear-down being a no-op. Ambly can deal with it, but a consequence is that it leaves an OS-level WebDAV mount running when the user exits the REPL.

Is the no-op of -tear-down intentional? (I took a quick peek at the code, but didn't spend the time to grok what was going on.)

cemerick commented 9 years ago

The -tear-down being a no-op in DelegatingREPLEnv is intentional and necessary to circumvent the default REPL env lifecycle in cljs.repl/repl*. The "real" teardown happens here.

But, I see that I think I'm not dereferencing through the delegating env. If not, I'll patch and cut a new snapshot shortly.

cemerick commented 9 years ago

Yeah, REPL environments weren't being torn down at all. :-P Now they are, new 0.2.0-SNAPSHOT build out.

I don't see any obvious way for the double -setup thing behaviour to happen though…

mfikes commented 9 years ago

Cool. I think the double setup was really just a lack of robustness on Ambly's part---it was throwing as a result of the previous run not tearing down. That has now been fixed, so even if the user kills the JVM, Abbly will handle that case more reliably.

I'll check for tear-down being called. :) I think this ticket can be closed. :)

mfikes commented 9 years ago

I checked and tear-down is indeed being called with the new 0.2.0-SNAPSHOT build when you type :cljs/quit.