Skip to content
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

[NETBEANS-6458] A bogus Cyclic Reference issue is resolved by marking fields as volatile. #3635

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

entlicher
Copy link
Contributor

@entlicher entlicher commented Feb 18, 2022

In the GraalVM debugging tests (java/debugger.jpda.truffle) there was often a Cyclic reference issue from FolderInstance.
It turned out, that there was not really any cyclic reference, but as the field waitFor was not marked as volatile, the code sometimes did not see the new value updated by the creation task.
By marking the fields as volatile we assure that their updated values are seen by other threads.

@entlicher entlicher marked this pull request as draft February 18, 2022 22:46
@entlicher entlicher marked this pull request as ready for review February 19, 2022 10:24
@entlicher entlicher changed the title DO NOT MERGE: Added logging to explore NETBEANS-6458. [NETBEANS-6458] A bogus Cyclic Reference issue is resolved by marking fields as volatile. Feb 19, 2022
@entlicher entlicher added this to the NB14 milestone Feb 19, 2022
Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am usually ashamed when I look back at my old multithreaded code after all those years. This is the case as well. The code using FolderInstance was randomly failing for many years, should your change fix that, then you deserve tons of kudos, Martine!

Making as much fields final is the best option. Some of the fields however have to be mutable. There is some synchronize(this) guard for some and the question is whether more appropriate to guard more fields by this or make more fields volatile?

@entlicher entlicher merged commit c07fe19 into apache:master Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants