Add tests for context values with scope semantics #3516
Conversation
...ore/src/withMicrometerTest/java/reactor/core/publisher/ContextPropagationWithScopesTest.java
Show resolved
Hide resolved
|
|
||
| AtomicReference<ScopedValue> valueInsideFlatMap = new AtomicReference<>(); | ||
|
|
||
| try (ScopedValue.Scope v1scope1 = v1.open()) { |
There was a problem hiding this comment.
Do we wanna check as well that if one modifies Scope then nothing wrong happens?
There was a problem hiding this comment.
in what sense do you mean? modifying the v1scope1 or opening a new scope?
There was a problem hiding this comment.
I was thinking about 2 cases:
- one call scope close() manually (probably that would break everything and should be illegal 🧐)
- one opens a scope manually and then closes it manually somewhere else in the chain e.g.
java var scope = ... mono .doOnSubscibe(__-> scope.set(Scope.open())) .doAfterTerminate(__-> scope.get().close())
which should be probably legal and should not break anything (but I'm not sure)
There was a problem hiding this comment.
one call scope close() manually (probably that would break everything and should be illegal 🧐)
Yes, this is illegal and the behaviour should be undefined. Adding tests for it would become some sort of a specification.
one opens a scope manually and then closes it manually somewhere else in the chain e.g.
java var scope = ... mono .doOnSubscibe(-> scope.set(Scope.open())) .doAfterTerminate(-> scope.get().close())
which should be probably legal and should not break anything (but I'm not sure)
In this case it's also illegal in my opinion - the reactive chain is agnostic of Thread switches. Scopes are Thread-bound. If you open a Scope in one Thread, you must close it in the same Thread. Doing that using reactive operators is dangerous, I'd avoid adding a test that shows such a pattern.
|
Also, should not this be targeted for |
I actually updated that at the same time you started reviewing. The PR was in draft mode before 3.5.x existed, so it aimed for main initialy. |
I added a test for flatMap in a concurrent scenario to verify that appropriate scopes are created in child threads. |
|
@OlegDokuka I updated to the latest code from micrometer-metrics/context-propagation#123 which is now finalized. |
|
@chemicL this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to |
No description provided.