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 ThreadSPropagationChInterceptor for stacking #8735

Merged

Conversation

artembilan
Copy link
Member

Related SO thread: https://stackoverflow.com/questions/77058188/multiple-threadstatepropagationchannelinterceptors-not-possible

The current ThreadStatePropagationChannelInterceptor logic is to wrap one message to another (MessageWithThreadState), essentially stacking contexts. The postReceive() logic is to unwrap a MessageWithThreadState, therefore we deal with the latest pushed context which leads to the ClassCastException

  • Rework ThreadStatePropagationChannelInterceptor logic to reuse existing MessageWithThreadState and add the current context to its stateQueue.
    Therefore, the postReceive() will poll() the oldest context which is, essentially, the one populated by this interceptor before, according to the interceptors order
  • Fix AbstractMessageChannel.setInterceptors() to not modify provided list of interceptors
  • The new ThreadStatePropagationChannelInterceptorTests demonstrates the problem described in that mentioned SO question and verifies that context are propagated in the order they have been populated

Cherry-pick to 6.1.x & 6.0.x

Related SO thread: https://stackoverflow.com/questions/77058188/multiple-threadstatepropagationchannelinterceptors-not-possible

The current `ThreadStatePropagationChannelInterceptor` logic is to wrap one
message to another (`MessageWithThreadState`), essentially stacking contexts.
The `postReceive()` logic is to unwrap a `MessageWithThreadState`,
therefore we deal with the latest pushed context which leads to the `ClassCastException`

* Rework `ThreadStatePropagationChannelInterceptor` logic to reuse existing `MessageWithThreadState`
and add the current context to its `stateQueue`.
Therefore, the `postReceive()` will `poll()` the oldest context which is, essentially,
the one populated by this interceptor before, according to the interceptors order
* Fix `AbstractMessageChannel.setInterceptors()` to not modify provided list of interceptors
* The new `ThreadStatePropagationChannelInterceptorTests` demonstrates the problem
described in that mentioned SO question and verifies that context are propagated
in the order they have been populated

**Cherry-pick to `6.1.x` & `6.0.x`**
…e scenario.

Essentially, copy the state queue to a new decorated message
* Fix `BroadcastingDispatcher` to always decorate message, even if not `applySequence`
@artembilan artembilan marked this pull request as ready for review September 15, 2023 14:40
@artembilan
Copy link
Member Author

OK. Thinking about this one more time, I convince myself that this wrapping approach is not so good. Especially when we send the same message to several handlers like in case of publish-subscribe channel.
And it looks like the header approach like I take for Spring Security fix is much easier and more robust: spring-projects/spring-security#12532.

We may still have this ThreadStatePropagationChannelInterceptor but it should create a new message with the context populated into provided header.

Probably that should be done as a new feature just for current version since it is a bit breaking change and this PR still makes sense as bug fix for previous version.

@garyrussell garyrussell merged commit eafedaa into spring-projects:main Sep 18, 2023
1 check passed
garyrussell pushed a commit that referenced this pull request Sep 18, 2023
* Fix ThreadSPropagationChInterceptor for stacking

Related SO thread: https://stackoverflow.com/questions/77058188/multiple-threadstatepropagationchannelinterceptors-not-possible

The current `ThreadStatePropagationChannelInterceptor` logic is to wrap one
message to another (`MessageWithThreadState`), essentially stacking contexts.
The `postReceive()` logic is to unwrap a `MessageWithThreadState`,
therefore we deal with the latest pushed context which leads to the `ClassCastException`

* Rework `ThreadStatePropagationChannelInterceptor` logic to reuse existing `MessageWithThreadState`
and add the current context to its `stateQueue`.
Therefore, the `postReceive()` will `poll()` the oldest context which is, essentially,
the one populated by this interceptor before, according to the interceptors order
* Fix `AbstractMessageChannel.setInterceptors()` to not modify provided list of interceptors
* The new `ThreadStatePropagationChannelInterceptorTests` demonstrates the problem
described in that mentioned SO question and verifies that context are propagated
in the order they have been populated

**Cherry-pick to `6.1.x` & `6.0.x`**

* * Fix `ThreadStatePropagationChannelInterceptor` for publish-subscribe scenario.
Essentially, copy the state queue to a new decorated message
* Fix `BroadcastingDispatcher` to always decorate message, even if not `applySequence`

* * Fix unused import in the `BroadcastingDispatcher`

* * Fix unused import in the `ThreadStatePropagationChannelInterceptor`
garyrussell pushed a commit that referenced this pull request Sep 18, 2023
* Fix ThreadSPropagationChInterceptor for stacking

Related SO thread: https://stackoverflow.com/questions/77058188/multiple-threadstatepropagationchannelinterceptors-not-possible

The current `ThreadStatePropagationChannelInterceptor` logic is to wrap one
message to another (`MessageWithThreadState`), essentially stacking contexts.
The `postReceive()` logic is to unwrap a `MessageWithThreadState`,
therefore we deal with the latest pushed context which leads to the `ClassCastException`

* Rework `ThreadStatePropagationChannelInterceptor` logic to reuse existing `MessageWithThreadState`
and add the current context to its `stateQueue`.
Therefore, the `postReceive()` will `poll()` the oldest context which is, essentially,
the one populated by this interceptor before, according to the interceptors order
* Fix `AbstractMessageChannel.setInterceptors()` to not modify provided list of interceptors
* The new `ThreadStatePropagationChannelInterceptorTests` demonstrates the problem
described in that mentioned SO question and verifies that context are propagated
in the order they have been populated

**Cherry-pick to `6.1.x` & `6.0.x`**

* * Fix `ThreadStatePropagationChannelInterceptor` for publish-subscribe scenario.
Essentially, copy the state queue to a new decorated message
* Fix `BroadcastingDispatcher` to always decorate message, even if not `applySequence`

* * Fix unused import in the `BroadcastingDispatcher`

* * Fix unused import in the `ThreadStatePropagationChannelInterceptor`
@garyrussell
Copy link
Contributor

...and cherry-picked.

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