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 instrumentation of newly loaded classes #525

Merged
merged 10 commits into from
Feb 18, 2025

Conversation

eupp
Copy link
Collaborator

@eupp eupp commented Feb 12, 2025

Before this commit, the instrumentation would only be triggered for re-transformed or re-defined classes.
With this change, instrumentation is also applied to new classes loaded during Lincheck test execution.

eupp added 5 commits February 12, 2025 23:06
…med or re-defined classes)

Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp eupp requested a review from dmitrii-artuhov February 12, 2025 22:29
@eupp
Copy link
Collaborator Author

eupp commented Feb 12, 2025

This is a part of #505

Copy link
Collaborator

@dmitrii-artuhov dmitrii-artuhov left a comment

Choose a reason for hiding this comment

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

Two nits: I would remove the mentioned if-statements, but leaving them is also ok

Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
Copy link
Collaborator Author

eupp commented Feb 17, 2025

nit: retransformClasses on empty array is guaranteed no-op, so this if is exhaustive

I also thought it should be no-op, but unfortunately without the surrounding if statement I can observe NullPointerException thrown from retransformClasses on Java 8, leading to failed tests.

I don't know what exactly is the reason, could be some bug in ByteBuddy or Kotlin compiler (e.g., maybe it decides to convert vararg argument with 0 provided arguments to null instead of empty array). I did not dig further to be honest, as simply adding the if statement seem to solve the problem.

I've added a comment to the code about this problem.

@ndkoval
Copy link
Collaborator

ndkoval commented Feb 17, 2025

@eupp, should a test that verifies the fix be added to this PR?

Signed-off-by: Evgeniy Moiseenko <[email protected]>
@eupp
Copy link
Collaborator Author

eupp commented Feb 17, 2025

should a test that verifies the fix be added to this PR?

This issue manifests on the coroutines related test that I am going to add under #505.

I've tried to minimize the bug but so far with no success.
It seems that the problem can only manifest if several non-trivial conditions are satisfied, including some interplay of lazy class transformation logic and ignored sections.

Normally in most cases, the lazy instrumentation would be applied to a class, for example, before the object is created:

override fun beforeNewObjectCreation(className: String) = runInIgnoredSection {
    LincheckJavaAgent.ensureClassHierarchyIsTransformed(className)
}

// `ensureClassHierarchyIsTransformed` first calls `Class.forName(canonicalClassName)` to ensure class is loaded

So at the point when LincheckClassFileTransformer::transform is called, the class is already loaded.

But if some object is created inside an ignored section, or its creation is untracked by some other reason, the beforeNewObjectCreation injection will not be called, and the class will not be instrumented. In such cases, instrumenting the class when it is loaded could be the only opportunity to instrument it.

@eupp eupp requested a review from ndkoval February 18, 2025 20:39
@ndkoval ndkoval merged commit b03e042 into develop Feb 18, 2025
20 checks passed
@ndkoval ndkoval deleted the eupp/fix-loaded-classes-instrumentation branch February 18, 2025 20:40
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.

3 participants