Skip to content

Remove ThreadLocal from RequestHandlerRetryAdvice #8650

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

Merged

Conversation

artembilan
Copy link
Member

Related to #8644

With virtual threads it is recommended to stay away from ThreadLocal variables to avoid memory exhausting with too many virtual threads

  • Fix RequestHandlerRetryAdvice to transfer the message context via internal IntegrationRetryCallback implementation.
  • Cast to this IntegrationRetryCallback in a newly introduced internal IntegrationRetryListener to extract messageToTry and set it into a RetryContext ErrorMessageUtils.FAILED_MESSAGE_CONTEXT_KEY attribute
  • Deprecate a usage of an external RetryListener implementation of the RequestHandlerRetryAdvice

Related to spring-projects#8644

With virtual threads it is recommended to stay away from `ThreadLocal`
variables to avoid memory exhausting with too many virtual threads

* Fix `RequestHandlerRetryAdvice` to transfer the message context via internal
`IntegrationRetryCallback` implementation.
* Cast to this `IntegrationRetryCallback` in a newly introduced internal `IntegrationRetryListener`
to extract `messageToTry` and set it into a `RetryContext`
`ErrorMessageUtils.FAILED_MESSAGE_CONTEXT_KEY` attribute
* Deprecate a usage of an external `RetryListener` implementation of the `RequestHandlerRetryAdvice`
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

I don't believe this will make any difference.

The RetryTemplate stores the whole context in a ThreadLocal; see

// Make sure the context is available globally for clients who need
// it...
RetrySynchronizationManager.register(context);

This is to allow downstream code to access the context.

Perhaps spring-retry could be changed to store the context in a map, and only store a map key in the TL?

@artembilan
Copy link
Member Author

The RetryTemplate stores the whole context in a ThreadLocal

Right. There are also many other third-party libraries we rely on in our frameworks which heavily use synchronized and may have their own ThreadLocal.
Therefore I would say this virtual threads story is similar to reactive streams (or native images): everything or nothing.
But at the same time it didn't stop us to implement and support those features.

My goal over here is to follow virtual threads best practice in this code base.
Other projects we depend on may follow this recommendation or not.

Probably that RetrySynchronizationManager could make a gain from our context-propagation library where ThreadLocal is one of the choices: https://github.com/micrometer-metrics/context-propagation

@artembilan artembilan requested a review from garyrussell June 16, 2023 20:41
@garyrussell garyrussell merged commit 9e9bfd0 into spring-projects:main Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants