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

Provide properties to block threads by default (again) #8704

Closed
swiss-chris opened this issue Aug 16, 2023 · 9 comments · Fixed by #8706
Closed

Provide properties to block threads by default (again) #8704

swiss-chris opened this issue Aug 16, 2023 · 9 comments · Fixed by #8706

Comments

@swiss-chris
Copy link
Contributor

swiss-chris commented Aug 16, 2023

Expected Behavior

Users of Spring Integration can configure the new timeout defaults (for details, see below) globally instead of sprinkling new boilerplate timeout code throughout the application.

Here's an example of how such a configuration could look in application.properties (or yaml).

  • spring.integration.thread-blocking-timeout=-1s
  • spring.integration.thread-blocking-timeout-enabled=false

Current Behavior

According to https://github.com/spring-projects/spring-integration/wiki/Spring-Integration-6.0-to-6.1-Migration-Guide#do-not-block-by-default, the default timeout for various SI operations has been changed from disabled to 30s. This breaks multiple SI projects in our team/company.

Context

We have several Spring Integration projects which are negatively affected by this change. We currently have 2 possibilities to fix our code.

  • Either we look for every gateway() and every scatterGather() call and pre-emptively disable the timeout in all these places. Today we looked at an application with 28 gateway() calls. Disadvantage: Lots of boilerplate code -> not Spring-like.
  • Or we take the time to analyse each gateway() and each scatterGather() call and only disable the timeout where we have decided the call could take more than 30s. Disadvantage: requires more time to do.

Here's an example of the kind of code I'm talking about.

.scatterGather(
  scatterer -> scatterer
    .recipientFlow(flow -> flow
      .gateway(flow -> flow
        .gateway(subflow, 
          spec -> spec.replyTimeout(-1L)),
        spec -> spec.replyTimeout(-1L))
  gatherer -> gatherer.doSomething(),
  spec -> spec.gatherTimeout(-1L)
)
@swiss-chris swiss-chris added status: waiting-for-triage The issue need to be evaluated and its future decided type: enhancement labels Aug 16, 2023
@artembilan
Copy link
Member

I thought about that when answered your SO question: https://stackoverflow.com/questions/76876192/spring-integration-6-1-breaking-change-threads-no-longer-block-indefinitely.
Even if that looks for me suspicious that your timeouts are much longer than 30s, I'm OK to expose such a property to let end-user to override.

I think the name of the property will be: spring.integration.endpoints.defaultTimeout.
That is the name of global property here in Spring Integration: https://docs.spring.io/spring-integration/docs/current/reference/html/configuration.html#global-properties.

In Spring Boot it might be like spring.integration.endpoint.default-timeout. But that is different story and we will come back there when we done here.

@artembilan artembilan added this to the 6.2.0-M2 milestone Aug 16, 2023
@artembilan artembilan added in: core (EOL) for: backport-to-6.1.x and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Aug 16, 2023
@swiss-chris
Copy link
Contributor Author

swiss-chris commented Aug 16, 2023

Thanks Artem.

PS: We're using Spring Integration for some heavy lifting, reading over a hundred thousands rows from DB tables, transforming them, and then persisting them to other DB tables. These operations often take longer than 30 seconds. In other places we're making parallel calls to external systems, transforming the results and persisting. That too can take more than 30 seconds. Not sure how we could do this differently.

@artembilan
Copy link
Member

In asynchronous manner: fire-n-forget.
Then you can check their status somewhat later using some token (correlation key) as a reply to your gateway or scatter-gather.
But I guess that's too much to rework on your side.

@artembilan artembilan self-assigned this Aug 17, 2023
artembilan added a commit to artembilan/spring-integration that referenced this issue Aug 17, 2023
Fixes spring-projects#8704

The default timeout for requests and replies in the integration endpoints
is 30 seconds to avoid indefinite blocking in threads.
Sometime those 30 seconds is not enough.

* Introduce a `spring.integration.endpoints.defaultTimeout` global property
to allow overriding all the timeouts to desired value.
The negative number indicates an indefinite waiting time: similar to what
was there before introducing 30 seconds by default
garyrussell added a commit that referenced this issue Aug 22, 2023
* GH-8704: Add global property for `defaultTimeout`

Fixes #8704

The default timeout for requests and replies in the integration endpoints
is 30 seconds to avoid indefinite blocking in threads.
Sometime those 30 seconds is not enough.

* Introduce a `spring.integration.endpoints.defaultTimeout` global property
to allow overriding all the timeouts to desired value.
The negative number indicates an indefinite waiting time: similar to what
was there before introducing 30 seconds by default

* Fix language in docs

Co-authored-by: Gary Russell <[email protected]>

---------

Co-authored-by: Gary Russell <[email protected]>
@swiss-chris
Copy link
Contributor Author

I tried but didn't succeed in getting this to work. I opened a separate issue here: spring-projects/spring-integration-samples#361

@swiss-chris
Copy link
Contributor Author

Also, it seems like the java dsl sample coffee app suffers from a similar fate as our applications: spring-projects/spring-integration-samples#361

@artembilan
Copy link
Member

Unfortunately that looks like we have missed to expose that property via Spring Boot configuration: spring-projects/spring-boot#41477.

I will think about a workaround for you over weekend...

@wilkinsona
Copy link
Member

I think that a bean post-processor for the integrationGlobalProperties bean could be used to set the timeout:

@Bean
static BeanPostProcessor defaultEndpointTimeout() {
	return new BeanPostProcessor() {
		@Override
		public Object postProcessAfterInitialization(Object bean, String beanName) throws BeansException {
			if (IntegrationContextUtils.INTEGRATION_GLOBAL_PROPERTIES_BEAN_NAME.equals(beanName)) {
				((IntegrationProperties) bean).setEndpointsDefaultTimeout(-1);
			}
			return bean;
		}
	};
}

@artembilan
Copy link
Member

I thought about that as well, Andy.
There is another one which is shorter, but a bit awkward:

	@Bean
	String integrationGlobalPropertiesWorkAround(IntegrationProperties integrationProperties) {
		integrationProperties.setEndpointsDefaultTimeout(-1);
		return null;
	}

😄

Due to the way it is implemented now in Spring Boot, we don't have any other choices and unfortunately with that we cannot read props from the file.

Well, we can do like manual @Value("META-INF/spring.integration.properties") Resource and IntegrationProperties,parse() to override what Spring Boot auto-configures for us, but is it really less code for a workaround on just one singe missed property?

The proper fix we will discuss in respective Spring Boot issue.

@wilkinsona
Copy link
Member

IIRC, more recent versions of Framework (maybe only 6.2?) will reject null beans. It's also possible that the timeout will have been read before the @Bean method has set it. While more verbose, I'd recommend that folks use a BeanPostProcessor as it should be more robust.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants