The logic of the new filter looks really good! Most of my comments are minor.

Please let me know if you have any questions. Thanks!

Reviewed 30 of 30 files at r1.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 183 at r1 (raw file):

   will be applied to the decompressed buffer or the non-decompressed buffer. It
   is recommended to keep this enabled to protect against zip bomb attacks. */
#define GRPC_ARG_ENABLE_PER_MESSAGE_DECOMPRESSION_INSIDE_CORE \

Why make this optional? Why not simply change this unconditionally? Is there some value in retaining the current mechanism?

src/core/ext/filters/http/http_filters_plugin.cc, line 40 at r1 (raw file):

    &grpc_message_compress_filter, GRPC_ARG_ENABLE_PER_MESSAGE_COMPRESSION};

static optional_filter decompress_filter = {

Is there any reason that compression and decompression should happen in two separate filters? Why not combine them into one?

src/core/ext/filters/http/message_decompress/message_decompress_filter.h, line 1 at r1 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 1 at r1 (raw file):

/*

Please use C++-style comments.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 47 at r1 (raw file):

class CallData {
 public:
  CallData(const grpc_call_element_args& args)

explicit

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 67 at r1 (raw file):

  ~CallData() { grpc_slice_buffer_destroy_internal(&recv_slices_); }

  static void DecompressStartTransportStreamOpBatch(

Please make this non-static and provide a standalone wrapper function for use in the filter vtable.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 70 at r1 (raw file):

      grpc_call_element* elem, grpc_transport_stream_op_batch* batch);

  static void OnRecvInitialMetadataReady(void* arg, grpc_error* error);

The rest of these methods can be private.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 86 at r1 (raw file):

 private:
  grpc_core::CallCombiner* call_combiner_ = nullptr;

Always initialized by ctor, so does not need to be initialized to nullptr here.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 104 at r1 (raw file):

  // created using it.
  grpc_slice_buffer recv_slices_;
  grpc_core::ManualConstructor<grpc_core::SliceBufferByteStream>

It's not appropriate to use ManualConstructor<> in a C++ class, since it's really only intended for embedding C++ objects inside of C structs. If the goal is that this field may be unset, please use absl::optional<> instead.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 336 at r1 (raw file):

}

void DecompressDestroyChannelElem(grpc_channel_element* /*elem*/) { return; }

No need for return; the function can just be empty.

src/core/lib/surface/call.cc, line 204 at r1 (raw file):

      GRPC_STREAM_COMPRESS_NONE;
  /* Maximum size for uncompressed receive message in bytes. -1 for unlimited */
  int max_uncompressed_receive_message_length = -1;

Why is this needed? I don't see it actually being used anywhere, and I would prefer to avoid bloating the call stack size if we can avoid it.

test/core/end2end/tests/compressed_payload.cc, line 258 at r1 (raw file):

  {
    grpc_core::ExecCtx exec_ctx;

Are you sure this is safe? Channel args could include arbitrary pointer values that may need an ExecCtx to destroy themselves.

test/core/end2end/tests/workaround_cronet_compression.cc, line 354 at r1 (raw file):

  {
    grpc_core::ExecCtx exec_ctx;

Same question here: Is this safe?

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @markdroth and @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 183 at r1 (raw file):

The value is as Craig said that proxies might want to avoid compression/decompression.
Also, I'm not sure if anyone in the wild uses minimum stack but if we make this unconditional and move the decompression logic entirely from the surface to the filter, then decompression filter has to be part of the minimal stack too.

src/core/ext/filters/http/http_filters_plugin.cc, line 40 at r1 (raw file):

That's what I had started with myself, but later abandoned the thought. I'm trying to recall the reason but I think it was mostly due to structuring issues -

1. Easier to control enabling/disabling of the filter with the channel arg through existing logic in http_filter_plugins
2. If someone wants to disable the compression filter but still have decompression inside core.

src/core/ext/filters/http/message_decompress/message_decompress_filter.h, line 1 at r1 (raw file):

will do, but the style guide still allows both https://google.github.io/styleguide/cppguide.html#Comment_Style

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 1 at r1 (raw file):

will do, but the style guide still allows both https://google.github.io/styleguide/cppguide.html#Comment_Style

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 47 at r1 (raw file):

Done.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 67 at r1 (raw file):

Done.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 70 at r1 (raw file):

Done.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 86 at r1 (raw file):

removed

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 104 at r1 (raw file):

I am just using this class to avoid having to construct the object at construction time since SliceBufferByteStream has a parameterised constructor, and still just allocate the object once. Given the internal usage of ManualConstructor, it seems to be used more than just for embedding within C structs. The internal doc says that we should consider using std::aligned_storage directly.
I am also fine with using absl::optional here but that would have a larger memory requirement and also while taking the address, we would need to do &*stream_ which seems kind of weird. Using std::aligned_storage seems better if you feel strongly about this

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 336 at r1 (raw file):

Done.

src/core/lib/surface/call.cc, line 204 at r1 (raw file):

I was thinking of ways I could enforce limits on decompression to avoid mallocing in the bad actor case, but I can do that in a separate PR.

test/core/end2end/tests/compressed_payload.cc, line 258 at r1 (raw file):

Yes they could, but the channel args here are integer args, so it should be safe.

test/core/end2end/tests/workaround_cronet_compression.cc, line 354 at r1 (raw file):

Yes they could, but the channel args here are integer args, so it should be safe.

Reviewed 3 of 4 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 183 at r1 (raw file):

Wait -- is this channel arg intended to disable decompression completely, or is it intended to toggle between decompressing inside of core and decompressing outside of core? The description led me to assume the latter.

What I'm trying to say here is that I think we should no longer support the option of decompressing outside of core. I think we should support decompressing inside of core and we should support not decompressing at all for the proxy case.

I doubt that anyone actually uses the minimal stack, so I don't care too much about that. But I would say that we should be consistent about compression and decompression: the minimal stack should either include both of them or neither of them.

src/core/ext/filters/http/http_filters_plugin.cc, line 40 at r1 (raw file):

As I've mentioned elsewhere, I don't think we should support decompression outside of core.

Do we actually know of a use-case where someone would want to enable decompression without compression (or vice-versa)?

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 104 at r1 (raw file):

I do feel strongly that we should not use ManualConstructor<> for this purpose. That API is really designed only as a transition feature from C to C++, so it should not be used in code that has already been converted to C++.

If the goal here is to create the object only when needed, then absl::optional<> is the right solution. That is how this sort of thing is done in C++ code.

I would expect that absl::optional<> would be slightly larger, but only by the size of a bool. I'm fine with that.

If you don't like saying &*stream_, you can instead say &stream_.value().

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 67 at r2 (raw file):

  ~CallData() { grpc_slice_buffer_destroy_internal(&recv_slices_); }

 public:

This line is not needed; the public section already started above.

src/core/lib/surface/call.cc, line 204 at r1 (raw file):

I do think we should do that, but I don't think this requires a new top-level value. We should be able to access the existing setting via the service config.

Anyway, doing this in a separate PR sounds good.

Please wait for the update. I'll combine the compress and decompress filters. I'll let you know when I'm done

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @markdroth and @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 183 at r1 (raw file):

It's the latter with the option for not decompressing it at all if they want. (They should be able to just forward it as is.)

That makes sense.

src/core/ext/filters/http/http_filters_plugin.cc, line 40 at r1 (raw file):

I can't think of one. I'll combine the two

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 104 at r1 (raw file):

ack. Will do

Only a few minor comments remaining. Please let me know if you have any questions. Thanks!

Reviewed 9 of 25 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 176 at r3 (raw file):

/** Enable/disable support for per-message compression/decompression. Defaults
 * to 1, unless GRPC_ARG_MINIMAL_STACK is enabled, in which case it defaults to
 * 0. If disabled, decompression will be performed lazily by

I thought we decided not to support decompression in byte_buffer_reader anymore?

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

namespace grpc_core {
extern const grpc_channel_filter grpc_message_compress_filter;

Since we're moving this into the grpc_core namespace, let's give this a C++-style name.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 54 at r3 (raw file):

// For testing purposes, disables/enables decompression of incoming data. If
// disabled, decompression will instead be performed by the byte_buffer_reader.

I thought we decided not to support decompressing in byte_buffer_reader anymore?

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 56 at r3 (raw file):

// disabled, decompression will instead be performed by the byte_buffer_reader.
// By default, decompression is enabled and done inside this filter.
void TestOnlyDisableDecompression();

Why are these needed? If we have a channel arg to disable compression, why don't we just use that in tests?

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 46 at r3 (raw file):

namespace grpc_core {
namespace {
bool g_test_only_disable_decompression = false;

As per my comments elsewhere, I don't think this should be needed.

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 190 at r3 (raw file):

  grpc_closure start_send_message_batch_in_call_combiner_;
  // The fields below are only initialized when we compress the payload.
  // Keep them at the bottom of the struct, so they don't pollute the

Is this sentence still relevant?

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @markdroth and @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 176 at r3 (raw file):

We decided to have decompression and compression done together. This handles the case that the proxy doesn't want Core to either compress/decompress like a proxy. The byte_buffer_reader fallback is still needed for backward compatibility for example if a proxy decides to log the message.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

You mean for grpc_message_compress_filter? The style guide doesn't specify anything special for namespace wide names. Do you prefer something like g_grpc_message_compress_filter?

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 54 at r3 (raw file):

This would be fallback for if a proxy decides to log/read the message.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 56 at r3 (raw file):

This is just for testing and verifying the compressed message and the various options associated with it. We want compression enabled but decompression disabled so that we can verify the compressed bytes.

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 190 at r3 (raw file):

removed

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 104 at r1 (raw file):

Done.

src/core/ext/filters/http/message_decompress/message_decompress_filter.cc, line 67 at r2 (raw file):

Done

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @yashykt)

include/grpc/impl/codegen/grpc_types.h, line 176 at r3 (raw file):

It doesn't really seem reasonable to me for a proxy to both disable decompression and then decide to log the decompressed message. You can't both have your cake and eat it too.

I don't want to maintain two code-paths for decompression, and I think that continuing to support decompression in byte_buffer_reader defeats the purpose of this change.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

I meant to use something like MessageCompressionFilter.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 54 at r3 (raw file):

See my earlier response. I don't think we should support that.

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 190 at r3 (raw file):

I was only referring to the last sentence. You also deleted the previous sentence, which is still relevant.

Reviewable status: 6 of 32 files reviewed, 4 unresolved discussions (waiting on @markdroth)

include/grpc/impl/codegen/grpc_types.h, line 176 at r3 (raw file):

Done

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

Isn't that format just for class names?

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 54 at r3 (raw file):

Done.

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 46 at r3 (raw file):

outdated

src/core/ext/filters/http/message_compress/message_compress_filter.cc, line 190 at r3 (raw file):

outdated

This looks really good! All remaining comments are minor.

Please let me know if you have any questions.

Reviewed 1 of 4 files at r2, 23 of 29 files at r5, 3 of 5 files at r6, 1 of 1 files at r7.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @yashykt)

BUILD, line 1196 at r7 (raw file):

        "src/core/ext/filters/http/http_filters_plugin.cc",
        "src/core/ext/filters/http/message_compress/message_compress_filter.cc",
        "src/core/ext/filters/http/message_decompress/message_decompress_filter.cc",

Suggest just putting this in the same directory as the message_compress_filter.

include/grpc/impl/codegen/grpc_types.h, line 180 at r7 (raw file):

   Defaults to 1. If disabled, decompression will not be performed and the
   application will see the compressed message in the byte buffer. */
#define GRPC_ARG_ENABLE_PER_MESSAGE_DECOMPRESSION_INSIDE_CORE \

Let's just call this GRPC_ARG_ENABLE_PER_MESSAGE_DECOMPRESSION.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

This is effectively a class name. It's a C-style vtable, but it serves the same function as a class.

In any case, it looks like you've reverted this file back to C style, so I guess it's fine to leave this as-is.

test/core/end2end/tests/compressed_payload.cc, line 494 at r7 (raw file):

    GPR_ASSERT(response_payload_recv->type == GRPC_BB_RAW);
    gpr_log(GPR_ERROR, "%d", decompress_in_core);

Can you please structure things such that this parameter is logged as part of the test name logged in begin_test() above?

test/core/end2end/tests/workaround_cronet_compression.cc, line 404 at r7 (raw file):

        workaround_configs[i].user_agent_override, true);
    request_with_payload_template(
        config, "test_invoke_request_with_compressed_payload", 0,

Please change the test name to include the suffix "_compression_disabled".

Reviewable status: 10 of 31 files reviewed, 4 unresolved discussions (waiting on @markdroth)

BUILD, line 1196 at r7 (raw file):

Done.

include/grpc/impl/codegen/grpc_types.h, line 180 at r7 (raw file):

Done.

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

I see. That is interesting. In that case, we should be able to C++ize the filter stack mechanism itself. Singleton classes maybe?

test/core/end2end/tests/compressed_payload.cc, line 494 at r7 (raw file):

Done.

test/core/end2end/tests/workaround_cronet_compression.cc, line 404 at r7 (raw file):

Done.

Addressed the comments

Reviewable status: 10 of 31 files reviewed, 4 unresolved discussions (waiting on @markdroth)

Looks great!

Reviewed 21 of 21 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved

src/core/ext/filters/http/message_compress/message_compress_filter.h, line 51 at r3 (raw file):

It's a little more complicated than that for performance reasons (surprise!). I've got a couple of branches lying around with incomplete attempts at different approaches for doing this (one from Craig), but I haven't yet had time to pick one and get it finished.