quarkus-jfr can cause a NullPointerException
#44976
Comments
|
I think the problem is here: There are probably a lot more cases than just the 404 for which we don't start the event. My preference would be to not try to mimic Quarkus REST behavior but just test if the @chiroito can you check the above make sense and implement a fix ^ |
|
@andrewazores do you have the sample project you used to surface this problem? I would very much like to run it and see the issue in action because as @gsmet says, this issue is likely more general than just 404 handling |
|
@geoand I ran into this in cryostatio/cryostat#733 , but I have prepared a minimal reproducer here: |
|
Thanks! I'll have a look soon |
|
Your reproducer is great, thanks a lot @andrewazores! There is a quick and dirty fix for this, but there is also a way to make this for all HTTP requests and I prefer to handle it like this. |
|
After looking into it a little more here is the issue we are facing with the way these JFR events are currently designed:
My question to @andrewazores and @chiroito is whether it is valid in JFR to have an event created at some point and commited at another. If that is valid I can make Quarkus REST always create a start event, update it if the mapping was successful (and then commit it) or when no mapping was possible also commit it. |
I think the specifics of "at some point" are critical here. It's certainly valid for the event to be created, then some time to pass, and then the event to be committed. Even if the reference to that event gets passed around to different places it should be OK. What I don't know about is whether that's true if the event creation and commit might happen on different threads (physical or virtual). Maybe @roberttoyonaga would know? |
|
@geoand and @andrewazores Like Andrew says, its perfectly fine for the event to be created and committed in different places (different functions or after some time etc.). To my knowledge, it is not ok for events to be created and committed by different threads. The thread specified in the JFR event field "event thread" should be the one that begins and commits each event. However, it is fine for one thread to create and commit an event on behalf of another thread when that event type has a "thread" field in addition to a n "event thread" field. (some periodic events are like this). But even in that case it is a singe thread creating and committing the event. |
|
Thanks folks, very helpful information! |
That's what I was thinking. So for Quarkus purposes I think this has implications for the various processors and filters, and the behaviour of the Blocking annotation or Mutiny async. |
|
Right, we'll have to examine that |
|
@geoand @andrewazores The result of running the code: So the "event thread" field is populated by whichever thread called As far as I'm aware, this doesn't happen with any of the built-in events. But it seems like nothing is stopping you from doing this at the application level. |
|
Excellent, thanks a lot @roberttoyonaga ! |
|
Another example I threw together, trying to make sure it still works as expected with a little more concurrency and processing happening: https://github.com/andrewazores/jfr-multithread-processing/blob/main/Main.java Everything looks good there too. |
|
🙏 |
|
#45059 is what I have in mind |
Improve JFR integration in Quarkus REST
quarkus/extensions/jfr/runtime/src/main/java/io/quarkus/jfr/runtime/http/rest/ServerRecorder.java
Line 57 in 18110f8
This is happening during an integration test run, which looks like this:
The REST endpoint class is like this:
If I simply remove the
quarkus-jfrdependency from the project'spom.xml, then this test goes back to passing as normal (as in, the application responds with 415 rather than 500).Quarkus version:
3.15.2The text was updated successfully, but these errors were encountered: