Comments (13)
borkdude
commented on May 25, 2024
1
This is expected. An AOT-ed uberjar made with 21 will assume that virtual threads were available. That uberjar won't run on 17. Choose the lowest Java version possible to create an uberjar if you want to make it portable across JVM versions.
from http-kit.
ptaoussanis
commented on May 25, 2024
@rafd Hi Rafal, thanks for the reproducible example! 🙏
Michiel seems to be correct (thanks @borkdude!), I was under the mistaken impression that an uberjar would still be okay in this case.
I'm really sorry about the confusion! Will update the release notes to add a warning, and will make sure any future releases are similarly marked when necessary.
Thanks a lot for pinging about this! 👍
from http-kit.
ptaoussanis
commented on May 25, 2024
Release notes have been updated 👍
from http-kit.
rafd
commented on May 25, 2024
Looking at the code, it seems http-kit could check for this at runtime, with a delay-cached value (like it does with java-version) instead of baking it in with the if-compile macro. Is doing it the way it is motivated by performance?
Whatever the reason, I'm content with the release warning. Hopefully if someone else runs into this, they'll find this issue and spend less time confused. (I'm still angry at brew for auto-updating my java).
Thanks for your work on http-kit @ptaoussanis and @borkdude!
from http-kit.
borkdude
commented on May 25, 2024
Is doing it the way it is motivated by performance?
I'd say both performance and graalvm native-image compatibility (you can't do the runtime check there anymore)
from http-kit.
ptaoussanis
commented on May 25, 2024
Looking at the code, it seems http-kit could check for this at runtime, with a delay-cached value (like it does with java-version) instead of baking it in with the if-compile macro
I'm not aware of any obvious/straight-forward way of doing this at runtime, but happy to see an example if you have something in mind.
The purpose of the delay in java-version>= is just for performance- the function body could just as well be (>= (java-version) (long n)). What matters is that java-version>= is a function, so that it's checked at runtime.
That's something that can be checked at runtime easily enough, because the decisions we make based on the outcome don't influence code generation - just branching.
The case of virtual threads is a little different since it involves code generation. We essentially want something like (defn get-thread [] <body>) that returns a virtual thread when possible. It's this body that would contain the conditional compile. Without the conditional compile, it's not obvious to me how you'd access virtual threads when they're available.
May be missing something though.
from http-kit.
rafd
commented on May 25, 2024
Where is the code-generation? This seems to be the only place where the Virtual Thread support check is made, and it could just be a run-time if:
master...rafd:http-kit:16c99a4ee00da3965394764b0f2f0e8341b2ca1f
Sorry if I am misunderstanding.
from http-kit.
rafd
commented on May 25, 2024
Ah, I missed borkdude's comment re: graalvm. If it must be compile-time for graal, then it makes sense.
from http-kit.
ptaoussanis
commented on May 25, 2024
This seems to be the only place where the Virtual Thread support check is made, and it could just be a run-time if
Have you tried this code on Java < 21?
(fn [] (java.util.concurrent.Executors/newVirtualThreadPerTaskExecutor)) won't be allowed since that method won't exist. That's what I mean by code generation.
We're not just branching at runtime between two valid code paths - one path will be completely inexplicable to the compiler. Does that make sense?
Sorry if I am misunderstanding.
No worries, it's good to check! I made one mistake on this myself already - extra eyes never hurt!
Ah, I missed borkdude's comment re: graalvm. If it must be compile-time for graal, then it makes sense.
I believe the issue is more fundamental than that. Even if GraalVM weren't in the picture, we'd have the same problem. (Again, unless I'm missing something).
from http-kit.
rafd
commented on May 25, 2024
Gotcha. Facepalm. (I had installed my patched http-kit version on java 21 - which did work in a project on both 17 and 21, but not if http-kit was built on 17 and run in a project on 17).
So, I have another patch on offer (it's become more of an academic exercise at this point):
master...rafd:http-kit:948738ea303c8bdae596ff7c7e6f1a1e83a77f43
It uses reflection. But the reflection is done in a way that Graal is able to automatically detect -- at least, I was able to use native-image on my updated test repo without any extra config or problems.
But it does leave the question of performance. The getMethod is cached, so it's a question of how much overhead the invoke adds after the JVM has warmed up. The JVM may be able to optimize it out completely for the same reason graal is happy with it. (For babashka, it would presumably have 0 penalty).
I've already been bit by the original issue and learned from it, so I don't have much of a horse in this race anymore -- so I'm totally fine with it being a wont-fix.
from http-kit.
ptaoussanis
commented on May 25, 2024
So, I have another patch on offer (it's become more of an academic exercise at this point
I support the academic exercise, it's a useful case for some exploration 👍 Reflection would be the way to go if we really wanted to do this at runtime for some reason.
Note that performance doesn't actually matter much here since we're talking about code that runs only when creating a worker pool.
That'll usually only happen twice (once for client worker + once for server worker), certainly rarely - so reflection cost doesn't actually matter much. Likewise the delay wouldn't be too important so you could simplify to something like:
(let [<...>
new-virtual-pool
(try
(let [^java.lang.reflect.Method m
(.getMethod java.util.concurrent.Executors
"newVirtualThreadPerTaskExecutor" nil)]
(fn [] (.invoke m nil nil)))
(catch Throwable _ nil))]
<...>)I'm not familiar enough with GraalVM to say whether that'd cause any build problems without actually trying it - but I guess that should be fine. And performance shouldn't be an issue here either IMO, for the reason already discussed (this code runs rarely, usually just twice). Even if one cares about the extra 1-time startup cost for something like Babashka, this reflection's in the order of 0.0001 msecs.
Anyway, that's on the academic end.
Re: whether to actually consider such a change or not - I wouldn't be keen to take on the extra complexity since I just don't think it'd buy us much. It's generally a good idea anyway to be in the habit of AOTing against the oldest Java release you want to support (see Sean Corfield's comment for one example why).
And there may be other areas in http-kit where we already do (or will in future want to do) similar compile-time conditionals. Then we'd be forced to adopt the same complexity in those areas too to retain the marginal benefit.
So my 2c- release notes should definitely bring attention to such changes when they're introduced, but I'm also happy that that should be sufficient - and simpler.
Hope that seems reasonable?
from http-kit.
Related Issues (20)
- Issue preventing non-keep-alive benchmarks HOT 1
- Race condition in TimerService.scheduleTask HOT 2
- Ring websocket API support HOT 4
- Problem in native-image and HttpUtils when using virtual threads HOT 11
- 2.8.0-beta2 has CIDER dependencies HOT 3
- CURL and finagle failing to parse :set-cookies with '\n' HOT 13
- Unix socket benchmark HOT 1
- It will turn headers into camel format HOT 2
- logger-warn gets rebound HOT 1
- logger-warn and error-warn are passed in wrong order to HttpServer constructor HOT 1
- Requests which throw java.net.ConnectException may actually succeed HOT 3
- Request Map doesn't contain information about authority HOT 8
- http-kit v2.8.0-RC1 HOT 1
- http-kit v2.8.0 final HOT 1
- Consider adding `Content-Type: text/plain` to HTTP 500 response in `org.httpkit.server.HttpHandler#run` HOT 6
- v2.7.0 SNI change broke connections to plain IP addresses with SSLHandshakeException "Hostname or IP address is undefined." HOT 8
- Add options for encoding nested form and query params a la clj-http HOT 7
- WebTransport support HOT 2
- [Proposal][Client] Consider more idiomatic bridges with JVM async paradigms HOT 5
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from http-kit.