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

Fix Idea plugin integration with GPMC #551

Merged
merged 6 commits into from
Mar 5, 2025
Merged

Fix Idea plugin integration with GPMC #551

merged 6 commits into from
Mar 5, 2025

Conversation

avpotapov00
Copy link
Collaborator

In the PR #471 Move ideaPluginEnabled into the file scope lincheck-plugin compatibility was broken, as ideaPluginEnabled was calculated and assigned before the plugin could replace the result of the ideaPluginEnabled() method.
To keep caching and to give the debugger time to replace the return value, we make this property lazy, so it'll be calculated only on the first request.
In this case, the debugger has enough time to replace the function's return value.

@avpotapov00 avpotapov00 requested a review from eupp February 28, 2025 10:19
@avpotapov00 avpotapov00 self-assigned this Feb 28, 2025
@eupp
Copy link
Collaborator

eupp commented Feb 28, 2025

@ivandev0 it looks like that ideaPluginEnabled was computed too early (during static initialization?), before the debugger had the opportunity to hijack ideaPluginEnabled() call. Because of this, the Lincheck plugin was not working. This PR makes ideaPluginEnabled a lazy property to delay its computation. Could you please check that lazy does not defeat the optimization?

@eupp eupp requested a review from ivandev0 February 28, 2025 10:34
@ivandev0
Copy link
Collaborator

Checked, performance still looking good.

@@ -56,7 +56,7 @@ fun testFailed(
* We will call `ideaPluginEnabled` method only once
* and so on the plugin side the callback will be called also only once.
*/
internal val ideaPluginEnabled = ideaPluginEnabled()
internal val ideaPluginEnabled by lazy { ideaPluginEnabled() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please leave a comment when the debugger becomes able to replace the ideaPluginEnabled() result?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@avpotapov00 I'm closing the PR but please address the issue later

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is an actual bug on Java 17 With Debugger Plugin (Lincheck)

@eupp
Copy link
Collaborator

eupp commented Mar 3, 2025

It looks like the root cause of the issue was because beforeEvent() injection was not invoked for thread start/join events.
It resulted in exception being thrown on this line:

error("Create nextEventId $nextId but last read event is $lastVisited, last read value is $lastIdReturnedAsCurrent")

I fixed it. As a nice bonus, now navigation to thread start/join trace points also works as expected in the plugin.

Another issue was because of the initialization order of eventIdStrictOrderingCheck and ideaPluginEnabled properties.
Property ideaPluginEnabled was dependent on property eventIdStrictOrderingCheck, but was initialized before it, leading to a subtle bug.
I reordered the properties declaration order for it to match the initialization order.

It looks like after the initialization order is fixed there is no need in lazy.
At least, I tested the plugin locally and everything seems to work.

@ndkoval could you also please test the plugin?

@eupp eupp requested a review from ndkoval March 3, 2025 22:59
@eupp eupp changed the title Idea plugin ideaPluginEnabled property mady lazy-loaded Fix Idea plugin integration with GPMC Mar 5, 2025
@eupp eupp merged commit edef131 into develop Mar 5, 2025
20 checks passed
@eupp eupp deleted the fix/idea_plugin branch March 5, 2025 11:48
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.

4 participants