Why reply register a SignalHandler when connect to remote nREPL server #127

Open
chunlinyao opened this Issue Oct 16, 2013 · 8 comments

Projects

None yet

3 participants

@chunlinyao

Reply register a SignalHandler when connect to a remote nREPL server. this cause Signal class hold a function from Clojure and can not unload all Clojure related classes and CassLoaders. I think the INT signal only should be handled in the client side. why this code is required?

https://github.com/trptcolin/reply/blob/master/src/clj/reply/eval_modes/nrepl.clj#L246

I'am writing a project which inject a nREPL server into other java process by pid. I want to unload all Clojure related classes which is loaded by a custom ClassLoader, But I found Signal hold the GC root. register SignalHandler.SIG_DFL can fix it.

@trptcolin
Owner

That's a good question. I had to dig around in the git history a bit to remember the answer, and luckily my past self explained it in a commit message. Basically when I had nREPL running in the same process but different classloader, the whole process would exit on INT signals because of the default signal handler.

But based on your suggestion, I wonder if we could use SignalHandler.SIG_IGN instead and have everything work? http://srcrr.com/java/oracle/openjdk/6/reference/sun/misc/SignalHandler-source.html

@chunlinyao

Can we distinguish between the same process nREPL server and remote nREPL server, and only replace signal handler for the same process situation. Than we can let remote server keep default signal handlers. SignalHandler.SIG_IGN can make the classloader which running nREPL unloadable.

@tobias
Contributor
tobias commented Nov 19, 2013

@trptcolin - is this the issue you mentioned to me outside the cupcake place? :)

We're seeing the same issue for folks using reply to connect to an Immutant process. Ideally, it would be nice to know when we are connecting to a remote nrepl vs. back into the same process, but I don't see an easy way to do that.

@trptcolin
Owner

@tobias yep this is the one.

We could easily solve a subset of this problem: when REPLy itself is responsible for starting up the nREPL server. Since we know enough to start the server, we could omit the signal handler whenever we don't start the server.

It seems like that's not enough for the usual case, where lein starts up the nREPL server. But maybe with some investigation it'll turn out that either my memory of the initial problem (the server side exiting) is wrong, or things have changed enough to eliminate it. Otherwise, maybe an option when starting REPLy that lets it know the server is in the same process? Though that seems pretty hacky...

@trptcolin trptcolin added a commit that referenced this issue Feb 21, 2014
@trptcolin Don't add remote signal handler
Looks like something else has changed since
4497310 to make this unnecessary. Yay!

refs #127, 133
035de1e
@trptcolin
Owner

OK, I finally got a chance to look at this, and it looks like the reason I had this code there initially is no longer valid. Any help testing (especially on Windows) would be appreciated, but this does seems to be working for me on master.

@trptcolin
Owner

I can't find any evidence on my VM that ctrl-c actually works at all in a Windows lein repl, even before this change. For now I'm going to have to leave it to people who actually use Windows to try things out and submit issues/patches if things are broken.

@trptcolin trptcolin closed this Mar 7, 2014
@trptcolin trptcolin reopened this Jun 18, 2014
@trptcolin
Owner

I was wrong. This causes brokenness on lein 2.4.2 when running inside a project (works fine outside of one, which must have been where I tested):

clj16.core=> (while true (+ 1 2 3))
Exception in thread "Thread-3" clojure.lang.ExceptionInfo: Subprocess failed {:exit-code 130}
        at clojure.core$ex_info.invoke(core.clj:4403)
        at leiningen.core.eval$fn__4770.invoke(eval.clj:236)
[... etc ...]

So I'm re-opening this so I remember to address it, but I'm wondering if the fix just belongs in lein - since the nrepl server subprocess is what dies, and lein is responsible for spinning that up, maybe lein should either (a) disconnect the server subprocess from the STDIN stream somehow or (b) register a no-op SIGINT handler.

@trptcolin
Owner

Ah, we're already attempting to disconnect from STDIN for the server subprocess. So I guess doing (b) in leiningen itself is the best idea I've got.

@trptcolin trptcolin added a commit to technomancy/leiningen that referenced this issue Jun 18, 2014
@trptcolin Ignore SIGINT sent to an interactive repl server
Unfortunately, even though *pump-in* is false, the server subprocess
still gets the SIGINT when you hit <ctrl-c>. REPLy used to take care of
that by registering a handler itself, but it's a ClassLoader leak, as
discussed in trptcolin/reply#127, so I removed it. But now subprocess
servers crash instead of gracefully being interrupted.

Here we're careful only to register the process in the specific case
where we're responsible for both the input and the server.

Pro: REPLy clients avoid classloader leaks.
Con: All REPLy clients have to implement this same sort of handling, but
only (?) if they're running in a subprocess.
e7c2330
@trptcolin trptcolin referenced this issue in technomancy/leiningen Jun 18, 2014
Merged

Ignore SIGINT sent to an interactive repl server #1570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment