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

6895: Agent Instrumentation fails to emit events when the instrumented class has a different/custom ClassLoader #142

Closed
wants to merge 4 commits into from

Conversation

Josh-Matsuoka
Copy link
Collaborator

@Josh-Matsuoka Josh-Matsuoka commented Oct 27, 2020

Hi,

This PR addresses JMC-6895, which made it so that the agent instrumentation would silently fail and not emit events when a class being instrumented was loaded by a custom classloader and its' methods were reflectively invoked.

The following is the sequence of events that caused this bug:

  • When the class was initially loaded by the AppClassLoader the agent instruments it as we'd expect
  • When the class was later loaded by the custom classloader, that classloader's loadClass call chain would lead to the agent being called to transform again
    -- Since the transform had already been done once it was marked as complete and would not do anything
    -- When the code for the class was later run, it would not emit the events due to a ClassNotFoundException on the injected event classes as they were not visible to the custom classloader

This patch resolves this and makes sure instrumentation is done with the custom classloader and that the event classes are visible to it. I've also included an integration test replicating the behavior that caused the bug.


Progress

  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JMC-6895: Agent Instrumentation fails to emit events when the instrumented class has a different/custom ClassLoader

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jmc pull/142/head:pull/142
$ git checkout pull/142

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 27, 2020

👋 Welcome back jmatsuoka! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2020

⚠️ @Josh-Matsuoka a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master e2ea7611ef380785f4b32e16a92ebc4f228c6d48
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

@openjdk openjdk bot added the rfr label Oct 27, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 27, 2020

Webrevs

@thegreystone thegreystone self-requested a review Oct 27, 2020
@thegreystone
Copy link
Member

@thegreystone thegreystone commented Oct 27, 2020

Formatting errors. :) Please run mvn spotless:apply.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2020

@Josh-Matsuoka This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

6895: Agent Instrumentation fails to emit events when the instrumented class has a different/custom ClassLoader

Reviewed-by: hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 0ab3153: 5067: Content assist uses too much screen area

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@Josh-Matsuoka
Copy link
Collaborator Author

@Josh-Matsuoka Josh-Matsuoka commented Oct 28, 2020

/integrate

@openjdk openjdk bot closed this Oct 28, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

@Josh-Matsuoka Since your change was applied there has been 1 commit pushed to the master branch:

  • 0ab3153: 5067: Content assist uses too much screen area

Your commit was automatically rebased without conflicts.

Pushed as commit 31619d6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants