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

Remove VirtualField agent fallback to weak map #4241

Closed
trask opened this issue Sep 29, 2021 · 4 comments
Closed

Remove VirtualField agent fallback to weak map #4241

trask opened this issue Sep 29, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@trask
Copy link
Member

trask commented Sep 29, 2021

See discussion at #4218 (comment)

In particular, the benefit of avoiding the fallback is that it can lead to memory leaks if not careful (e.g. #441 and #3027).

One concern about removing the fallback was identified:

there is hack that skips field injection on classes annotated with javax.decorator.Decorator

I think my comment was not correct:

I wonder if the javax.decorator.Decorator hack is needed since #3948

because it looks like the javax.decorator.Decorator hack is just an ignore matcher.

@laurit just confirming that I understand your concern correctly, does the concern apply to everything in ignores matchers? and if that's the case, if we remove the fallback, wouldn't tests for any instrumentation that uses a VirtualField key that is in an ignore matcher fail?

@laurit
Copy link
Contributor

laurit commented Sep 29, 2021

I meant

this is separate from ignores matcher. I don't think ignore matcher applies to field injection.
If we decide to remove the fallback then we have to keep in mind that there are classes that don't have the field. We add field to all subclasses of Runnable. Thread is a Runnable, but doesn't have the fields because it is loaded too early.

@laurit
Copy link
Contributor

laurit commented Sep 30, 2021

Field injection could also fail when hasSuperType matcher can't find some super types. This happens when getResourceAsStream of class loader that defines the class can't find bytes for some of the super types. It could be that this class loader doesn't implement getResourceAsStream in a sensible way or defined class has a generated super type (ClassLoader.getResourceAsStream won't find anything for generated classes) or in osgi it should be possible to have class hierarchies where class sees its immediate super types but not their super types.

@trask
Copy link
Member Author

trask commented Aug 8, 2022

@laurit do you think there's anything to be done here, or should we close it?

@laurit
Copy link
Contributor

laurit commented Aug 8, 2022

@trask I think we should close this. There are classes that are loaded before field injection is set up and rely on the fallback to function properly, for example java.util.concurrent.FutureTask.
To find classes that rely on fallback you can print out the key in

and run executor tests.

@trask trask closed this as completed Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants