On Thu, 13 Nov 2025 03:28:51 GMT, Prasanta Sadhukhan <[email protected]> 
wrote:

>> NPE is seen while accessing transient "scenePeer" variable between reads..
>> Fix is made to store it in a temp variable rather than reading it twice 
>> since the value can change between successive reads in many places it is 
>> accessed.
>> Also some debug logs added to be enabled via `jfxpanel.debug` property
>
> Prasanta Sadhukhan has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - debug log fix
>  - debug log fix

Implementing a system with shared state that is concurrently accessed by 
different threads is difficult, and it requires rigorous analysis because it's 
almost impossible to empirically verify that code is _not_ racy. Neither the 
original implementation nor the changes in this PR inspire confidence that this 
analysis has happened. It seems like we are presented yet again with a supposed 
fix without a clear explanation of why this is the right approach.

Just by looking at `JFXPanel` I can see that `stagePeer` is accessed in the 
same unsafe way as `scenePeer`. Now, I don't suggest that it can be "fixed" by 
making local copies of the reference, this might just as well obfuscate the 
problem even further. As it stands, I think this patch should not be integrated 
into JavaFX before an appropriate analysis of the problem has been done. An 
appropriate analysis is not pointing out that "an NPE happens because another 
thread set a shared field to `null`"; this is just a consequence of a racing 
system that the JVM managed to elucidate because it checked the reference for 
us.

As a side note, I don't understand the point of all the logging. If these 
methods have thread invariants (which you imply in your 
[comment](https://github.com/openjdk/jfx/pull/1968#discussion_r2521176030)), 
then enforce those invariants. If they don't, what's the point?

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1968#issuecomment-3529248960

Reply via email to