Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent #3693

alexander-yakushev · 2024-05-31T14:38:59Z

Also, add cider-enable-nrepl-jvmti-agent customizable to control this behaviour.

For now, the required Java property is injected for Leiningen and Clojure CLI jack ins. It probably should also do so for gradle and maven?

Additionally, I need advice on where to document this best, and if this can be tested somehow.

You've added tests (if possible) to cover your change(s)
You've updated the changelog (if adding/changing user-visible functionality)
You've updated the user manual (if adding/changing user-visible functionality)

vemv · 2024-05-31T20:55:41Z

cider.el

-                               lein-middlewares))
+                               lein-middlewares)
+                      (when cider-enable-nrepl-jvmti-agent
+                        `(,(concat "update-in :jvm-opts conj " (shell-quote-argument "-Djdk.attach.allowAttachSelf")))))


Lein update-in tends to play poorly with everything else (plugins, profiles, etc) IME - I wish it worked as one would imagine, but quite commonly it doesn't.

We already use enrich-classpath for adding misc JVM flags. In that sense, that project slightly grew in scope from "replace the classpath with some entries attached at the tail" to also "add whatever flags are needed for cider-nrepl to work".

I'd suggest adding the flag there, and have fewer code paths here.

(It's also consistent with the current state of things - enrich-classpath is not a third-party thing but our only approach to have misc key features working)

I don't think that's a good idea, as enrich-classpath is optional functionality and the interrupt functionality is kind of central to the eval workflows. From my perspective it'd be best to keep them separate, although I agree we should be careful where/how exactly we set the :jvm-opts.

Well, it's optional but in the end it brings features that every JVM Clojure dev wants (namely those that have lived in Orchard for many years but had no way of being used).

We're not too far away from being in a position of enabling it by default.

In specific terms, Enrich adds the following flags, smartly, only for further easing stuff in Orchard:

--add-opens=jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED

--add-opens=jdk.compiler/com.sun.tools.javac.code=ALL-UNNAMED

As you can imagine, either asking users to add that by hand or adding it in cider are both less practical/maintainable.

With that said, we already use update-in in a lot of places, so I don't mind a lot.

I'll reiterate here that we should not make assumptions that people are using CIDER with cider-nrepl (even when it comes to cider-jack-in. That's why I like to keep whatever is at the nREPL core level separate from CIDER's additional functionality.

bbatsov · 2024-06-01T06:01:22Z

For now, the required Java property is injected for Leiningen and Clojure CLI jack ins. It probably should also do so for gradle and maven?

Yeah, that would be ideal. This might also be a good time to purge the Boot stuff.

Additionally, I need advice on where to document this best, and if this can be tested somehow.

The documentation can go here I guess - https://docs.cider.mx/cider/usage/code_evaluation.html#configuration

I think there were a few tests checking that the params for Lein and friends were properly generated, so you can add something there.

I'm not sure where's the best place to add this exactly, but your proposal seems reasonable.

alexander-yakushev · 2024-06-03T04:48:50Z

Added tests, updated docs. Ready for review.

bbatsov · 2024-06-03T05:51:43Z

doc/modules/ROOT/pages/usage/code_evaluation.adoc

+=== Enable evaluation interrupts on Java 21 and newer
+
+Special variable `cider-enable-nrepl-jvmti-agent` has to be enabled for
+interrupts to work properly on Java 21+. See


In the other section you've written Java 20+, so I'm guessing one of the two is not correct. :-)

Yeah, I know I sowed confusion with this one. I have to pick one. Technically, it's JDK20+, but since JDK20 is not supported by CIDER, and in a year everyone will forget what 20 is, perhaps it makes sense to write 21 as the first LTS version where this is needed? What do you think?

Let's go with Java 21.

bbatsov · 2024-06-03T05:59:33Z

doc/modules/ROOT/pages/basics/up_and_running.adoc

+Since version 1.2.0, nREPL ships together with a native JVMTI agent, so that the
+eval interrupts properly work on Java 20 and later. To enable the agent, the
+Java process should be launched with `-Djdk.attach.allowAttachSelf`. CIDER will
+do it automatically during jack-in if you change


"... if/when cider-enable-nrepl-jvmti-agent is set to t." would read better IMO.

bbatsov · 2024-06-03T06:00:07Z

doc/modules/ROOT/pages/basics/up_and_running.adoc

+==== Enabling nREPL JVMTI agent
+
+Since version 1.2.0, nREPL ships together with a native JVMTI agent, so that the
+eval interrupts properly work on Java 20 and later. To enable the agent, the


bbatsov · 2024-06-03T06:01:36Z

doc/modules/ROOT/pages/usage/code_evaluation.adoc

@@ -161,6 +161,12 @@ NOTE: CIDER internally increases the timeout to 30 seconds for the first sync ev

 == Configuration

+=== Enable evaluation interrupts on Java 21 and newer
+
+Special variable `cider-enable-nrepl-jvmti-agent` has to be enabled for


"Special -> The configuration variable" might read better here.

Add `cider-enable-nrepl-jvmti-agent` customizable to control this behaviour.

yuhan0 · 2024-06-02T20:12:57Z

cider.el

            ;; TODO: global-options are deprecated and should be removed in CIDER 2.0
            (if global-options (format "%s " global-options) "")
+            (if cider-enable-nrepl-jvmti-agent "-J-Djdk.attach.allowAttachSelf " "")


Late to the review, but are there any practical differences between injecting it this way as a command-line arg vs. adding a :jvm-opts key to the inlined :cider/nrepl alias?

No difference at all. They will not conflict either if set in both places.

Do you mean this could be put into the alias instead of a top-level property? Yeah, we could probably do that. Can be changed in the future easily.

Yeah, and since we're now going the route of injecting jvm-opts to clojure-cli jack ins, it might be worth revisiting the decision back in #3023 and see if the -XX:-OmitStackTraceInFastThrow flag could also be added.

alexander-yakushev changed the title ~~Allows adding -Djdk.attach.allowAttachSelf to jack-in params~~ Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent May 31, 2024

alexander-yakushev force-pushed the jvmti-agent branch from 4b78af9 to 5d7d866 Compare May 31, 2024 15:13

vemv reviewed May 31, 2024

View reviewed changes

alexander-yakushev force-pushed the jvmti-agent branch 9 times, most recently from f770352 to 1c07576 Compare June 3, 2024 04:48

bbatsov reviewed Jun 3, 2024

View reviewed changes

alexander-yakushev force-pushed the jvmti-agent branch from 1c07576 to 83a9413 Compare June 3, 2024 06:33

Support adding -Djdk.attach.allowAttachSelf to jack-in params

b4935ff

Add `cider-enable-nrepl-jvmti-agent` customizable to control this behaviour.

alexander-yakushev force-pushed the jvmti-agent branch from 83a9413 to b4935ff Compare June 3, 2024 06:57

bbatsov merged commit 49b1527 into master Jun 3, 2024
39 checks passed

bbatsov deleted the jvmti-agent branch June 3, 2024 07:21

yuhan0 reviewed Jun 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent #3693

Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent #3693

alexander-yakushev commented May 31, 2024 •

edited

Loading

vemv May 31, 2024

bbatsov Jun 1, 2024

vemv Jun 1, 2024 •

edited

Loading

vemv Jun 1, 2024

bbatsov Jun 2, 2024

bbatsov commented Jun 1, 2024

alexander-yakushev commented Jun 3, 2024

bbatsov Jun 3, 2024

alexander-yakushev Jun 3, 2024

bbatsov Jun 3, 2024

alexander-yakushev Jun 3, 2024

bbatsov Jun 3, 2024

bbatsov Jun 3, 2024

bbatsov Jun 3, 2024

yuhan0 Jun 2, 2024 •

edited

Loading

alexander-yakushev Jun 3, 2024

alexander-yakushev Jun 3, 2024

yuhan0 Jun 3, 2024 •

edited

Loading

Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent #3693

Support adding -Djdk.attach.allowAttachSelf to jack-in params to enable nrepl JVMTI agent #3693

Conversation

alexander-yakushev commented May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vemv Jun 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jun 1, 2024

alexander-yakushev commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhan0 Jun 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuhan0 Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

alexander-yakushev commented May 31, 2024 •

edited

Loading

vemv Jun 1, 2024 •

edited

Loading

yuhan0 Jun 2, 2024 •

edited

Loading

yuhan0 Jun 3, 2024 •

edited

Loading