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 possible type pollution in ConditionEvaluationReport #32916

Closed

Conversation

dreis2211
Copy link
Contributor

@dreis2211 dreis2211 commented Oct 29, 2022

Hi,

I've been playing around with the https://github.com/RedHatPerf/type-pollution-agent from @franz1981 and found one case in Spring-Boot that caught my eye. On a vanilla app startup the output shows the following:

--------------------------
Type Pollution Statistics:
--------------------------
1:	org.springframework.beans.factory.support.DefaultListableBeanFactory
Count:	350
Types:
	org.springframework.beans.factory.config.ConfigurableListableBeanFactory
	org.springframework.beans.factory.config.ConfigurableBeanFactory
	org.springframework.beans.factory.BeanFactory
	org.springframework.beans.factory.HierarchicalBeanFactory
	org.springframework.beans.factory.ListableBeanFactory
	org.springframework.beans.factory.support.BeanDefinitionRegistry
	org.springframework.beans.factory.config.SingletonBeanRegistry
Traces:
	org.springframework.boot.autoconfigure.condition.ConditionEvaluationReport.find(ConditionEvaluationReport.java:171)
		class: org.springframework.beans.factory.config.ConfigurableListableBeanFactory
		count: 156
	org.springframework.boot.autoconfigure.condition.ConditionEvaluationReport.find(ConditionEvaluationReport.java:170)
		class: org.springframework.beans.factory.config.ConfigurableBeanFactory
		count: 150

On another much much bigger app of mine it's still TOP 13:

13:     org.springframework.beans.factory.support.DefaultListableBeanFactory
Count:  3795
Types:
        org.springframework.beans.factory.config.ConfigurableBeanFactory
        org.springframework.beans.factory.config.ConfigurableListableBeanFactory
        org.springframework.beans.factory.HierarchicalBeanFactory
        org.springframework.beans.factory.ListableBeanFactory
        org.springframework.beans.factory.BeanFactory
        org.springframework.beans.factory.support.BeanDefinitionRegistry
        org.springframework.beans.factory.config.AutowireCapableBeanFactory
        org.springframework.beans.factory.config.SingletonBeanRegistry
Traces:
        org.springframework.boot.autoconfigure.condition.ConditionEvaluationReport.find(ConditionEvaluationReport.java:171)
                class: org.springframework.beans.factory.config.ConfigurableListableBeanFactory
                count: 1635
        org.springframework.boot.autoconfigure.condition.ConditionEvaluationReport.find(ConditionEvaluationReport.java:170)
                class: org.springframework.beans.factory.config.ConfigurableBeanFactory
                count: 1566

Imho, the (unlikely but) possible pollution here doesn't really matter in terms of performance and is neglectable. (At least I didn't see an improvement or regression in some measurements) What's weird about this is that ConditionEvaluationReport.find does an instanceof check on ConfigurableBeanFactory but casts to ConfigurableListableBeanFactory.

We have two options here, I think:

  • Aligning the cast ConfigurableBeanFactory
  • Check for ConfigurableListableBeanFactory with instanceof

Ultimately, I chose the former for this PR , because everything that ConditionEvaluationReport.get internally needs seems to be satisfied with ConfigurableBeanFactory and thus seems like the more appropriate type. But since this changes a public method, we might want to go for the instanceof check on ConfigurableListableBeanFactory instead.

Let me know what you think.

Cheers,
Christoph

@wilkinsona
Copy link
Member

Thanks, @dreis2211. Irrespective of the possible type pollution, there's a bug at the moment as the cast will fail in the (unlikely) event that the ConfigurableBeanFactory isn't a ConfigurableListableBeanFactory. I would prefer to fix that by changing the instanceof rather than the public API. That will allow us to fix the bug in 2.6.x without the burden of keeping the public API backwards compatible which would require deprecating get(ConfigurableListableBeanFactory) and introducing get(ConfigurableBeanFactory) as a replacement. This deprecation and replacement would require quite a number of casts in calling code to avoid deprecation warnings.

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Nov 1, 2022
@dreis2211
Copy link
Contributor Author

@wilkinsona Adjusted the PR accordingly.

@wilkinsona wilkinsona added type: task A general task and removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged labels Nov 1, 2022
@wilkinsona wilkinsona added this to the 2.6.x milestone Nov 1, 2022
@wilkinsona wilkinsona self-assigned this Nov 8, 2022
@wilkinsona wilkinsona modified the milestones: 2.6.x, 2.6.14 Nov 8, 2022
@wilkinsona
Copy link
Member

Thanks very much, @dreis2211.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants