Add missing subscribeShareContext to missing static sources #1812
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: The subscribeShareContext was added in a few cases when defer was used but not added to all return paths. It should also be added to static sources like succeed and failed for consistency. Modifications: - Update deprecated ContentCoding filters to use subscribeShareContext on static succeeded operators. - Move subscribeShareContext in tracing filters to the usage of defer, to make the need for subscribeShareContext more clear. Result: More consistent usage of subscribeShareContext on static sources when control flow otherwise expects sharing context.
Scottmitch
requested review from
bondolo,
chemicL,
tkountis and
idelpivnitskiy
tkountis
approved these changes
Sep 8, 2021
chemicL
approved these changes
Sep 8, 2021
|
Let's hold this PR until after we discuss this on team sync. I'm playing with your example and think it may not represent what actually happens in these filters. |
idelpivnitskiy
approved these changes
Sep 10, 2021
There was a problem hiding this comment.
I had another look here, and realized what you were talking about. Effectively, there will be no difference for the AsyncContext state for users, because there is no user code involved when we return a static source. But I'm agreed that we should be consistent in case of errors as well, and not make a copy of the context. It will also remove one more allocation, which is nice. Thank you for the PR!
Scottmitch
added a commit
that referenced
this pull request
Sep 21, 2021
Motivation: The subscribeShareContext was added in a few cases when defer was used but not added to all return paths. It should also be added to static sources like succeed and failed for consistency. Modifications: - Update deprecated ContentCoding filters to use subscribeShareContext on static succeeded operators. - Move subscribeShareContext in tracing filters to the usage of defer, to make the need for subscribeShareContext more clear. Result: More consistent usage of subscribeShareContext on static sources when control flow otherwise expects sharing context.
|
0.41 (58c6e1d) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
The subscribeShareContext was added in a few cases when defer was used
but not added to all return paths. It should also be added to static
sources like succeed and failed for consistency.
Modifications:
on static succeeded operators.
to make the need for subscribeShareContext more clear.
Result:
More consistent usage of subscribeShareContext on static sources when
control flow otherwise expects sharing context.