Skip to content

grpc_json: use a mutex to protext JsonTranscoderConfig::type_helper_ #18894

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.

Sign up for GitHub

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

Closed
qiwzhang wants to merge 1 commit into from
Closed

grpc_json: use a mutex to protext JsonTranscoderConfig::type_helper_ #18894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,8 +344,19 @@ ProtobufUtil::Status JsonTranscoderConfig::createTranscoder(

for (const auto& binding : variable_bindings) {
google::grpc::transcoding::RequestWeaver::BindingInfo resolved_binding;
status = type_helper_->ResolveFieldPath(*request_info.message_type, binding.field_path,
&resolved_binding.field_path);
{
/**
* type_helper_ is not a thread safe object. Its used
* `TypeInfoForTypeResolver
* <https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/util/internal/type_info.cc#L170>`_
* uses three mutable maps to cache used data. This function is called in the worker thread.
* Multiple worker threads may use type_helper concurrently and these maps could be corrupted.
* Hence use a mutex to protect it.
*/
absl::MutexLock lock(&type_helper_mutex_);
status = type_helper_->ResolveFieldPath(*request_info.message_type, binding.field_path,
&resolved_binding.field_path);
}
if (!status.ok()) {
if (ignore_unknown_query_parameters_) {
continue;
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "source/common/protobuf/protobuf.h"
#include "source/extensions/filters/http/grpc_json_transcoder/transcoder_input_stream_impl.h"

#include "absl/synchronization/mutex.h"
#include "google/api/http.pb.h"
#include "grpc_transcoding/path_matcher.h"
#include "grpc_transcoding/request_message_translator.h"
Expand Down Expand Up @@ -126,6 +127,7 @@ class JsonTranscoderConfig : public Logger::Loggable<Logger::Id::config>,

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_;
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Nov 4, 2021

Choose a reason for hiding this comment

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

Can you add a locked by annotation on this object to make sure we hit all uses now and in the future?

Copy link
Copy Markdown
Contributor Author

@qiwzhang qiwzhang Nov 4, 2021

Choose a reason for hiding this comment

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

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.

grpc-ecosystem/grpc-httpjson-transcoding#64

Protobuf::util::JsonPrintOptions print_options_;

Expand Down