(maint) Update clj-parent to 5.1.1; fips config; dynapath; fix core reflection #94
Conversation
Exception in thread "main" java.lang.IllegalArgumentException: Must hint overloaded method: toArray, compiling:(flatland/ordered/set.clj:19:1)
Fixes Execution error (ClassNotFoundException) at java.net.URLClassLoader/findClass (URLClassLoader.java:476). org.bouncycastle.asn1.x500.X500Name
...since the behavior has changed.
Previously lein test would produce: WARNING: An illegal reflective access operation has occurred WARNING: Illegal reflective access by dynapath.defaults$eval29028$fn__29029 to method java.net.URLClassLoader.addURL(java.net.URL) WARNING: Please consider reporting this to the maintainers of dynapath.defaults$eval29028$fn__29029 WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations WARNING: All illegal access operations will be denied in a future release
For example, hashing and equality are blocking operations (they require network lookups). From https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/net/URL.html#equals(java.lang.Object): Since hosts comparison requires name resolution, this operation is a blocking operation.
I originally did this because I misread a flame graph and thought I was seeing notable reflection in tk status, but only afterward did I realize it was actually, lower down, just that i18n reflection problem we've already fixed. Since the changes were trivial, might be worth keeping.
Fixes:
ERROR in (auth-service-ssl-status-endpoint-test) (SSLIOSession.java:265)
Uncaught exception, not in assertion.
expected: nil
actual: javax.net.ssl.SSLException: java.lang.IllegalArgumentException: TLSv1.3
There was a problem hiding this comment.
The changes mostly look good to me!
Some of the commit messages could be clearer though, in my opinion:
- The
thing: actual changeformat is a bit weird to me. I find it clearer to say something likeMade change in thing. Does that make sense? - The multi-line error commit could use a little more info - the behavior of what changed? when?
- I would find it useful for the reflection commit to explain why the change is still worthwhile, but that could just be me not knowing anything about reflection.
| @@ -16,6 +16,7 @@ | |||
| :dependencies [[org.clojure/clojure] | |||
|
|
|||
| [cheshire] | |||
| [org.tcrawley/dynapath "1.0.0"] | |||
There was a problem hiding this comment.
Looks like we have this in clj-parent: https://github.com/puppetlabs/clj-parent/blob/7d37cd237cbbe8e93b461483b1d95c286829800c/project.clj#L91
Should we just bump the version there?
There was a problem hiding this comment.
I'm not sure, I think maybe Austin told me that there was some existing issue with puppetserver that would break if we did, and the fix hasn't been prioritized yet? It's currently also pinned in pdb, to fix the same issue, fwiw.
Ahh, don't feel strongly about it, but I'm used to using either in various projects, and I sometimes favor the former to either preserve space on the summary line, or to keep a series of related changes to a given subsystem easier to spot in a
Hmm, which one? You mean the
It's mostly just that reflection can be very expensive if it's ever involved in fine-grain enough use (the canonical example might be a tight loop), and so it's never desirable, but the annotations add "noise"/maint, and so might not be worth it if there are a lot of them in code that's always coarse grain. Here the changes were trivial enough that I was leaning toward "won't hurt, might help", since I'd already done them, but I can just drop that commit if we prefer. |
| (java.util.concurrent CancellationException) | ||
| (java.lang.management ManagementFactory) | ||
| (javax.management ObjectName) | ||
| (clojure.lang IFn))) | ||
|
|
||
|
|
||
| (set! *warn-on-reflection* true) |
There was a problem hiding this comment.
Do we want this turned on in the shipping code?
There was a problem hiding this comment.
No strong opinion - I'd been thinking that either we'd keep the file clean now that it's clean, or, we'd decide we didn't want to at some point later, and we'd remove the line then. I don't think it should have any affect in production otherwise, offhand?
There was a problem hiding this comment.
(Oh, and to be clear, also happy to just take that out.)
|
@rbrw Is this something you're still interested in landing? If so, it could use some updates to the clj-parent version and the bouncycastle jdk15on -> jdk18on change. |
|
|
I started this because I'd noticed what looked like notable reflection cost in some pdb flame graphs while working on escalations, and I thought it'd be easy to fix. Several yaks later, this pr.
It also turned out that I'd misread the graph, and that further down, the real culprit was the i18n reflection problem that we've fixed in newer versions, so we don't have to keep the status-core reflection fixes, but I thought they might be worthwhile since they're fairly straightforward and not too noisy.
See the individual commit messages for specific details.