grpc_json: use a mutex to protext JsonTranscoderConfig::type_helper_#18894
Closed
qiwzhang wants to merge 1 commit intoenvoyproxy:mainfrom
Closed
grpc_json: use a mutex to protext JsonTranscoderConfig::type_helper_#18894qiwzhang wants to merge 1 commit intoenvoyproxy:mainfrom
qiwzhang wants to merge 1 commit intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
mattklein123
requested changes
Nov 4, 2021
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks I agree this is an OK fix. One small comment, thanks.
/wait
| Protobuf::DescriptorPool descriptor_pool_; | ||
| google::grpc::transcoding::PathMatcherPtr<MethodInfoSharedPtr> path_matcher_; | ||
| mutable absl::Mutex type_helper_mutex_; | ||
| std::unique_ptr<google::grpc::transcoding::TypeHelper> type_helper_; |
Member
There was a problem hiding this comment.
Can you add a locked by annotation on this object to make sure we hit all uses now and in the future?
Contributor
Author
There was a problem hiding this comment.
It is too complicate to protect all type_helper_. its resolver is already thread-safe. Only its type_info_ is not.
I changed code here to make type_info thread safe.
Contributor
Author
|
Abandon this changes. Instead of using this |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Signed-off-by: Wayne Zhang qiwzhang@google.com
To fix #18849
JsonTranscoderConfig::type_helper_is not a thread safe object. Its usedTypeInfoForTypeResolver
uses three mutable maps to cache used data.
Most of places use
type_helper_are in theJsonTranscoderConfigconstructor which is only called at main thread.But there is a place inside
JsonTranscoderConfig::createTranscoderand this function is called per-request processing in the worker thread. It may have race condition that multiple threads try to update the mutable maps.Hence use a mutex to protect it.
Tried alternative approach: creating
type_helper_as thread local. It did not workgrpc_json_transcoder_integration_test failed at this
GOOGLE_CHECK as
Did not spend too much time to debug it. Maybe descriptor should not be used in the worker threads.
As low risk, just add a mutex to protect it. This
ResolveFieldPathshould be fairly quick, so the contention should be low, it should not slow down much.Risk Level: None
Testing: unit tests and integration tests
Docs Changes: None
Release Notes: None