This page is not created by, affiliated with, or supported by Slack Technologies, Inc.
2024-05-22
Channels
- # announcements (17)
- # beginners (11)
- # biff (5)
- # calva (22)
- # cider (30)
- # clj-kondo (33)
- # clj-on-windows (20)
- # clojure (59)
- # clojure-dev (25)
- # clojure-europe (31)
- # clojure-nl (1)
- # clojure-norway (13)
- # clojure-sweden (5)
- # clojure-uk (6)
- # clojurescript (5)
- # community-development (2)
- # cursive (4)
- # datahike (5)
- # datalevin (7)
- # datomic (11)
- # emacs (8)
- # events (1)
- # gratitude (1)
- # hoplon (5)
- # hyperfiddle (1)
- # lsp (59)
- # matrix (11)
- # polylith (14)
- # portal (3)
- # practicalli (1)
- # rdf (2)
- # reitit (9)
- # releases (3)
- # rum (5)
- # yamlscript (6)
Dear CIDER users, I once again ask for your help to test this feature out 🚀. The instructions are https://clojurians.slack.com/archives/C0617A8PQ/p1715068371996009, but you no longer have to build nREPL yourself. (edited)
I've been following this issue and am looking forward to it. I have a question about whether the feature will be opt in. In the issue I see:
> It has to be decided if such functionality (if it ever merges) has to be protected by an additional feature flag – for folks that don't want to sideload binary agents into their Java process. Resolution: allowAttachSelf can act as an opt-in flag to use the feature.
and
> For dynamic native agent loading, the JVM has to be started with -Djdk.attach.allowAttachSelf. This is the biggest hurdle from the user's perspective. At the same time, this can act as the opt-in flag for using the agent. Resolution: allowAttachSelf also acts as an opt-in, and CIDER will be able to inject this.
In other places, it says that sideloading this agent should be opt-in (which I agree with), but it's unclear from these statements whether that will be the case. Additionally, there are other reasons for including allowAttachSelf
besides enabling this particular agent.
Yes, we have decided that allowAttachSelf is a sufficient gate to opt-in into this.
So it's not opt-in.
Given that the combination of factors is already required for the agent to be loaded (allowAttachSelf + Java 21 + user has to interrupt + the thread has to continue working after Thread.interrupt()), it felt redundant to add another confirmation.
I don't think that's an unlikely scenario. All of those behaviors are except allowAttachSelf
are common.
However, the most likely reason for a user to have allowAttachSelf
is not to enable this particular feature.
Most of the time, when someone includes allowAttachSelf
, it's to enable some other feature.
CIDER will have an explicit opt-in that adds allowAttachSelf property. But if the user enables that on their own, they will get the same behavior.
I don't see why allowAttachSelf
should automatically opt in for the feature. Every single usage of allowAttachSelf
that currently exists is unrelated to this feature.
Because it is required for the feature to work, and it has to be set anyway. This was discussed in the issue.
I understand that explicit opt in will add that flag, but I don't see why adding that flag would opt-in for sideloading this agent.
Because that's what was proposed and agreed upon, and because that made most sense convenience-wise.
Is there an explicit opt out feature?
ie. If someone explicitly does not want this agent loaded, but still uses allowAttachSelf
, is there anyway to disable that?
It appears that this feature can not be opted out of if allowAttachSelf
is set.
No opt out at the moment (except for, you know, (.bindRoot #'nrepl.middleware.session/jvmti-stop-thread (fn [& _])
, but that's not really a user friendly toggle:)). But it can be added if necessary.
Do you have immediate practical reasons to avoid loading this agent but load other ones (don't trust the code, want to prevent crashes, etc.) or is it more on the holistic side (there is no adequate way to disable this)?
It's not uncommon to enable allowAttachSelf
in dev (see https://grep.app/search?current=3&q=allowAttachSelf&filter[lang][0]=edn). I sometimes do it myself. If the goal is to make this feature opt-in, I don't think simply checking allowAttachSelf
is sufficient.
https://github.com/nextjournal/clerk/blob/ee0248bbc1054476e781e25b11414fa9899050b9/deps.edn#L40
Yes, I know, I see most of those examples are accompanied by clj-async-profiler, by the way, so it's used to be able to load native agents:).
I think the opt-out is easy to do and makes sense. I think the "soft opt-in" via allowAttachSelf makes enough sense for most people and outweighs the necessity for a separate opt-in.
In the end, the agent is minimalistic and should never cause problems once we test it enough
So, gradually, opting out of it would be less and less necessary as it's still a fundamental nREPL feature.
I've been following the feature on github. From what I could tell, making the feature opt in seemed like an explicit goal so I didn't follow up on it more. The use of this feature does introduce more failure modes that aren't typically possible. I haven't had a chance think all of them, but trying to understand all the implications is tricky, which I thought was the reasoning behind making the feature opt-in.
It's possible and and maybe even probable that it doesn't matter much either way.
I'm pretty sure that it won't matter, but thank you for bringing this up. It goes without saying that a single bug/crash report would be enough for me to rethink this approach. And I will probably add the opt-out.
Hi Oleksandr, thanks for working on this. I tested this on Ubuntu 22.04.4 LTS with the following JDK:
openjdk version "21.0.2" 2024-01-16
OpenJDK Runtime Environment (build 21.0.2+13-Ubuntu-122.04.1)
OpenJDK 64-Bit Server VM (build 21.0.2+13-Ubuntu-122.04.1, mixed mode, sharing)
It seems to be working fine. I think there is a mistake in the instructions to try it out. The variable to customize is called cider-injected-nrepl-version
, and NOT cider-nrepl-injected-version
.
I did the following:
- Clone the nREPL repository
- Run lein install
and noted that 1.2.0-beta1
got installed
- Evaluate (setq cider-injected-nrepl-version "1.2.0-beta1")
in Emacs
And then tried the (while true)
test. It works as expected.
I also tried interrupting a test like this:
(deftest test-interrupt
(while true))
The test gets interrupted, but I think we have an issue with the test reporter not being able to handle this. I reported this here:
https://github.com/clojure-emacs/cider/issues/3677
This was already broken before and doesn't seem to be related to your changes.Not sure if the context around this PR and linked issues might help, re. opting in / out of injected JVM flags https://github.com/clojure-emacs/cider/pull/3042