Skip to content

INT-3661: Fix the eager BF access from BPPs (P I) #1393

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

Closed
wants to merge 4 commits into from

Conversation

artembilan
Copy link
Member

JIRA: https://jira.spring.io/browse/INT-3661

Status Quo

Previously there were a lot of noise from the PostProcessorRegistrationDelegate$BeanPostProcessorChecker for early access for beans.
That may produce some side-effects when some of BeanFactoryPostProcessors won't adjust those beans.

The issue is based on two facts:

  1. Loading beans from BPP, e.g. IntegrationEvaluationContextAwareBeanPostProcessor (or ChannelSecurityInterceptorBeanPostProcessor - https://jira.spring.io/browse/INT-3663)
  2. Loading beans from setBeanFactory()/setApplicationContext() container methods

The fix

  • Move all stuff from setBeanFactory() with access to the BeanFactory (e.g. this.messageBuilderFactory = IntegrationUtils.getMessageBuilderFactory(this.beanFactory);)
    to some other lazy-load methods like getMessageBuilderFactory()
  • Fix parser tests to assertNotSame for messageBuilderFactory since there is no activity for target components to lazy-load the stuff
  • Polish some test according the new lazy-load logic
  • Rework IntegrationEvaluationContextAwareBeanPostProcessor to the SmartInitializingSingleton and make it Ordered
  • Populate beanFactory for the internal instance of connectionFactory in the TcpSyslogReceivingChannelAdapter
  • Populate beanFactory for the internal UnicastReceivingChannelAdapter in the UdpSyslogReceivingChannelAdapter
  • Add log.info that UdpSyslogReceivingChannelAdapter overrides outputChannel for the provided UnicastReceivingChannelAdapter
  • Change the internal MessageChannel in the UdpSyslogReceivingChannelAdapter to the FixedSubscriberChannel for better performance

@artembilan
Copy link
Member Author

Also fixes https://jira.spring.io/browse/INT-3662

private volatile MessageBuilderFactory messageBuilderFactory;
private volatile MessageBuilderFactory messageBuilderFactory = new DefaultMessageBuilderFactory();

private volatile boolean messageBuilderFactorySet;
Copy link
Member Author

Choose a reason for hiding this comment

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

This class has a bug in the afterPropertiesSet() will be fixed tomorrow

@artembilan
Copy link
Member Author

Pushed

JIRA: https://jira.spring.io/browse/INT-3661

##Status Quo

Previously there were a lot of noise from the `PostProcessorRegistrationDelegate$BeanPostProcessorChecker` for early access for beans.
That may produce some side-effects when some of `BeanFactoryPostProcessor`s won't adjust those beans.

The issue is based on two facts:
1. Loading beans from `BPP`, e.g. `IntegrationEvaluationContextAwareBeanPostProcessor` (or `ChannelSecurityInterceptorBeanPostProcessor` - https://jira.spring.io/browse/INT-3663)
2. Loading beans from `setBeanFactory()/setApplicationContext()` container methods

## The fix

* Move all stuff from `setBeanFactory()` with access to the `BeanFactory` (e.g. `this.messageBuilderFactory = IntegrationUtils.getMessageBuilderFactory(this.beanFactory);`)
to some other lazy-load methods like `getMessageBuilderFactory()`
* Fix parser tests to `assertNotSame` for `messageBuilderFactory` since there is no activity for target components to lazy-load the stuff
* Polish some test according the new lazy-load logic
* Rework `IntegrationEvaluationContextAwareBeanPostProcessor` to the `SmartInitializingSingleton` and make it `Ordered`
* Populate `beanFactory` for the internal instance of `connectionFactory` in the `TcpSyslogReceivingChannelAdapter`
* Populate `beanFactory` for the internal `UnicastReceivingChannelAdapter` in the `UdpSyslogReceivingChannelAdapter`
* Add `log.info` that `UdpSyslogReceivingChannelAdapter` overrides `outputChannel` for the provided `UnicastReceivingChannelAdapter`
* Change the internal `MessageChannel` in the `UdpSyslogReceivingChannelAdapter` to the `FixedSubscriberChannel` for better performance
* Add JavaDocs for the `IntegrationEvaluationContextAware`
@artembilan
Copy link
Member Author

Pushed fix for MongoDbMessageStoreClaimCheckIntegrationTests. I haven't had a local MongoDB before, hence all tests have been bypassed by Rule reason 😄

@@ -57,7 +58,7 @@ public void interceptor() {
TestUtils.getPropertyValue(channel, "dispatcher"), "maxSubscribers", Integer.class).intValue());
channel = context.getBean("pubSub", MessageChannel.class);
Object mbf = context.getBean(IntegrationUtils.INTEGRATION_MESSAGE_BUILDER_FACTORY_BEAN_NAME);
assertSame(mbf, TestUtils.getPropertyValue(channel, "dispatcher.messageBuilderFactory"));
assertNotSame(mbf, TestUtils.getPropertyValue(channel, "dispatcher.messageBuilderFactory"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes no sense to me; the goal was to use the global MBF for all message builds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I understand your original goal. But I wanted to pay your attention here that we now is in the lazy-load, hence these tests really don't make sense because there is no anymore any interaction.
Are you OK if I remove these assertNotSame with the next commit/polishing to this PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see; you're not saying that at runtime we're using a different builder just that, since these are parser only tests, we won' have it yet.

I agree; they should just be removed.

However, we might want to consider adding verification to runtime tests instead 😄

(That can be a separate PR if you prefer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I also intend to remove them.
Re. separate JIRA: no need, really. See PublishSubscribeChannelParserTests changes. I hope that should prove that the fix works as promised.

@garyrussell
Copy link
Contributor

Wow - a lot of repetitive, thankless work (but I thank you !!).

@garyrussell
Copy link
Contributor

Nothing more from me.

@artembilan
Copy link
Member Author

Pushed

@garyrussell
Copy link
Contributor

Merged as 2bde14b

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.

2 participants