-
-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
orchard.java/class-info class initialization breaks JavaFX #250
Comments
Maybe we could somehow provide a banlist of packages which class-info should skip? |
Yes, please refer to #235 (comment) for the suggested approach If that's too much of an ask, we can also hardcode that specific class for now - that class never works anyway so a more comprehensive fix would be superset of the smaller fix. Thanks! |
Maybe we can hardcode skip the javafx package and go for a more general solution if there is ever another problem with this? I feel it is a super niche issue to introduce a new file. Wdyt? |
I don't know - there are all sorts of exotic Java classes so a file would seem best to me. That, or a System property edn-encoding a list of classes, but it would seem cumbersome. |
So list of classes wouldn't work unless they are prefixes of some kind. On javafx for example almost any class can break it (any control class at least) |
it seems fine to accept packages and classes, separately e.g. orchard.edn with |
I figured that for JavaFX adding dev/user.clj with : (ns user
(:import [javafx.application Platform]))
(Platform/startup (fn [] (println "JavaFX toolkit initialized"))) solves this issue. Which is a better solution for me than banning it since I get the class info for all javafx packages and classes. |
Awesome! Let's leave this issue open for a while since users can encounter it independently and it won't be obvious to them what's going on. Suggestions welcome. Are all cider+flowstorm users affected? |
Perhaps we should also document the JavaFX workaround in the README (e.g. under "Caveats")? |
This wasn't affecting FlowStorm users, just developing FlowStorm with the latest cider was crashing. |
Yes. Although it's also true that cider-nrepl breaks altogether, which we should prevent from happening in the first place. If we don't come up with something better (e.g. dyamically, non-obstrusively detecting whether Javafx has been initialized), I'd prevent analysis of that package unless the user has said it has initialized it (e.g. pass a System prop, as a hint that initialization is there). Maybe we can try |
I don't know if we should invest too much efforts into this given how niche JavaFX is these days. Might be easiest to just hardcode a workaround for this or something along those lines. |
If I understand correctly, the problem happens because cider-nrepl tries to initialize classes that the user didn't ask it to. Regardless of whether JavaFX is to blame here, I can imagine other instances where the user wouldn't want classes being initialized out of order, and passing So, how important warming those caches is? If it only costs +0.5 seconds for the user on first access versus potentially breaking something, I'd say that this UX optimization is not worth it. Or, at least, limit the warmup to JDK base and Clojure classes. Or is there something else that will stop working if the warmup doesn't happen? |
Hi, sorry, I'd really appreciate the discussion not going off rails. I've already spent a lot of time leaving this in a usable and performant manner for a wide variety of use cases. Now all that's being discussed is the flavor of small hardcoding to be applied. The long-term solution is #211 and I have set time in my calendar to specifically work on it May and June. Thanks - V |
Just wondering, does it require |
Yes, without Enrich, Orchard won't have access to javadocs that can be parsed (btw, Enrich had some bumpy beginnings but I'd encourage you to try it out - I hear good user reports from a variety of people and much fewer bug reports. The inspector also becomes better with those javadocs) |
I also hit this when trying to test one of the cljfx examples.
The exception seems to indicate that JavaFX does not like to be initialized on a thread which is not the "event thread":
Adding a |
Currently you can't develop JavaFX applications with the latest versions of Cider because orchard.java/class-info tries to initialize JavaFX classes without the toolkit already initialized. More of a problem is that initializing any JavaFX class before the toolkit has been initialized breaks JavaFX state, so just catching the Exception leaves the system broken.
Changing that line into :
so it doesn't initialize the class doesn't fix the issue, because there is code further down the road that will "touch" the class and initialize it anyway.
Here is a minimal repo with instructions that doesn't run with Cider https://github.com/jpmonettas/cider-nrepl-bug-repro/
The text was updated successfully, but these errors were encountered: