C++ify grpc_server by vjpai · Pull Request #22757 · grpc/grpc

vjpai · 2020-04-23T20:12:15Z

Followup from the review comments in #22670

This converts the grpc_server and other structures in the same file to idiomatic C++ (*). This also converts the parts of grpc_tcp_server and grpc_udp_server that borrow directly from the grpc_server's structures into idiomatic C++ (but doesn't change the rest of those structures, following the 1 CL=1 concept rule).

The readability is much more "normal" now (no ->next all over the place to create our own linked lists instead of using std::list or std::vector) and there is no performance impact.

(*) One piece of the core server that is not fully idiomatic is the map in channel_registered_methods, which is still implemented using an array (now a vector) rather than a map of some kind. This detail may be performance-sensitive so it is being pushed to a later CL after proper investigation.

This change is

vjpai · 2020-04-27T15:08:35Z

Sending for review as I believe all tests now should be passing except for the sanity failure currently reported at #22796

markdroth

Thank you both for improving our code health and for giving me a rare opportunity to make a relevant meme! :)

https://memegen.corp.google.com/5903205085741056

Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @vjpai)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 52 at r1 (raw file):

namespace {

struct server_state;

This code should also be completely converted to C++ (e.g., structs converted to classes with C++-style names, standalone functions changed into methods, all code moved into the grpc_core namespace). But if you don't want to tackle that now, that's fine.

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 225 at r1 (raw file):

  server_state* state = static_cast<server_state*>(arg);
  grpc_core::RefCountedPtr<grpc_core::HandshakeManager> handshake_mgr;
  {

This would be a bit cleaner if written to avoid ReleasableMutexLock by moving the code that requires the lock into a helper function:

grpc_core::RefCountedPtr<grpc_core::HandshakeManager> CreateHandshakeManager(server_state* state) {
  grpc_core::MutexLock lock(&state->mu);
  if (state->shutdown) return nullptr;
  grpc_resource_user* resource_user =
      grpc_server_get_default_resource_user(state->server);
  if (resource_user != nullptr &&
      !grpc_resource_user_safe_alloc(resource_user,
                                     GRPC_RESOURCE_QUOTA_CHANNEL_SIZE)) {
    gpr_log(
        GPR_ERROR,
        "Memory quota exhausted, rejecting the connection, no handshaking.");
    return nullptr;
  }
  auto handshake_mgr = grpc_core::MakeRefCounted<grpc_core::HandshakeManager>();
  handshake_mgr->AddToPendingMgrList(&state->pending_handshake_mgrs);
  grpc_tcp_server_ref(state->tcp_server);
  return handshake_mgr;
}

Then the code here becomes:

auto handshake_mgr = CreateHandshakeManager(state);
if (handshake_mgr == nullptr) {
  grpc_endpoint_shutdown(tcp, GRPC_ERROR_NONE);
  grpc_endpoint_destroy(tcp);
  gpr_free(acceptor);
  return;
}

src/core/lib/iomgr/tcp_server_utils_posix.h, line 86 at r1 (raw file):

  /* all pollsets interested in new connections */
  const std::vector<grpc_pollset*>* pollsets;

Is this struct being created/destroyed with C++ new/delete or with C malloc/free? If it's going to include a C++ type, we should make sure it's the former.

src/core/lib/iomgr/udp_server.cc, line 192 at r1 (raw file):

  /* all pollsets interested in new connections */
  const std::vector<grpc_pollset*>* pollsets;

Same question here about new/delete vs malloc/free.

src/core/lib/surface/server.h, line 75 at r1 (raw file):

  virtual void OnStart(const std::vector<grpc_pollset*>* pollsets) = 0;

  // OnDestroy indicates that the server should stop listening.

Instead of having an OnDestroy() method, why not just have the server destroy the listener, and trigger whatever shutdown behavior is needed via the dtor? This would require the destroy_done closure to be set earlier (maybe via the ctor?), but that seems doable.

src/core/lib/surface/server.h, line 84 at r1 (raw file):

   and when it shuts down, it will call destroy */

void grpc_server_add_listener(

Since these methods are not part of the C-core public API, they can be in C++ idiom, not C idiom. So why not expose the definition of grpc_server and just make these methods of that object?

In fact, I suspect most of the APIs in this file can be nested inside of the grpc_server type.

src/core/lib/surface/server.h, line 85 at r1 (raw file):

void grpc_server_add_listener(
    grpc_server* server, grpc_core::ListenerCallbackInterface* callbacks,

What's the ownership model of ListenerCallbackInterface? I was expecting this to take a unique_ptr<> and delete the instance when the listener is destroyed.

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

void server_recv_trailing_metadata_ready(void* user_data, grpc_error* error);

struct listener {

Pretty much everything in this file should be moved to the grpc_core namespace and changed to use C++ idioms (e.g., standalone functions replaced with methods, use C++ naming conventions, etc). Most of this code can probably be nested inside of grpc_server.

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

void server_recv_trailing_metadata_ready(void* user_data, grpc_error* error);

struct listener {

Seems a little strange to have a separate struct for this. Why not just make this the same as ListenerCallbackInterface and have the caller pass in a new instance?

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

  size_t cq_idx;
  std::list<channel_data*>::iterator list_position;
  bool listed;

I assume this indicates whether list_position is set? If so, suggest just making list_position an absl::optional<>.

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

  }

  grpc_channel_args* const channel_args;

Data members should be private and have a _ suffix.

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

  std::vector<listener> listeners;
  size_t listeners_destroyed = 0;
  grpc_core::RefCount internal_refcount;

Can we make this inherit from InternallyRefCounted<>, so that we can use RefCountedPtr<> to manage the refs?

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

  void FillChannelsLocked(const grpc_server* s) {
    GPR_DEBUG_ASSERT(channels_.empty());
    for (const channel_data* chand : s->channels) {

Would it help performance to do channels_.reserve(s->channels.size()) here?

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

      /* this was the first queued request: we need to lock and start
         matching calls */
      grpc_core::ReleasableMutexLock lock(&server_->mu_call);

I generally am not a big fan of ReleasableMutexLock; I think in most cases where it is used, the code would be clearer if restructured to use a normal MutexLock. I think this is a good example of that. As an alternative, consider moving the code that requires the lock into a helper method like this:

struct RequestedCallEntry {
  requested_call* rc;
  call_data* calld;  
};

RequestedCallEntry GetNextRequestedCall() {
  grpc_core::MutexLock lock(&server_->mu_call);
  if (pending_.empty()) return {nullptr, nullptr};
  RequestedCallEntry entry;
  entry.rc = reinterpret_cast<requested_call*>(
      requests_per_cq_[request_queue_index].Pop());
  if (entry.rc == nullptr) return {nullptr, nullptr};
  entry.calld = pending_.front();
  pending_.pop_front();
  return entry;
}

Then the loop here becomes the following:

while (true) {
  RequestedCallEntry entry = GetNextRequestedCall();
  if (entry.rc == nullptr) break;
  if (!gpr_atm_full_cas(&entry.calld->state, PENDING, ACTIVATED)) {
    // Zombied Call
    GRPC_CLOSURE_INIT(
        &entry.calld->kill_zombie_closure, kill_zombie,
        grpc_call_stack_element(grpc_call_get_call_stack(entry.calld->call), 0),
        grpc_schedule_on_exec_ctx);
    grpc_core::ExecCtx::Run(DEBUG_LOCATION, &entry.calld->kill_zombie_closure,
                            GRPC_ERROR_NONE);
  } else {
    publish_call(server_, entry.calld, request_queue_index, entry.rc);
  }
}

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

    /* no cq to take the request found: queue it on the slow list */
    GRPC_STATS_INC_SERVER_SLOWPATH_REQUESTS_QUEUED();
    grpc_core::ReleasableMutexLock lock(&server_->mu_call);

Similar comment here: Please see if there's a cleaner way to avoid ReleasableMutexLock.

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

 private:
  grpc_server* const server_;
  std::list<call_data*> pending_;

std::list<> does have to allocate a small amount of memory for each node in the list. Is there a performance impact of this change?

test/core/surface/concurrent_connectivity_test.cc, line 97 at r1 (raw file):

  grpc_server* server;
  grpc_completion_queue* cq;
  std::vector<grpc_pollset*> pollset;

Same question as elsewhere regarding new/delete vs. malloc/free.

test/core/util/test_tcp_server.h, line 32 at r1 (raw file):

  int shutdown;
  gpr_mu* mu;
  std::vector<grpc_pollset*> pollset;

Same question as elsewhere regarding new/delete vs. malloc/free.

vjpai

Thanks for the review! WRT meme links in reviews, please consider using public sites since this is an OSS project. WRT your specific template:
. I'll respond to a few comments but expect to do most of the work next week (or perhaps a little later).

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

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 52 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This code should also be completely converted to C++ (e.g., structs converted to classes with C++-style names, standalone functions changed into methods, all code moved into the grpc_core namespace). But if you don't want to tackle that now, that's fine.

Sounds good, I'll do this. It'll probably come through next week since I have some other priority items, but might as well do it right.

src/core/lib/iomgr/tcp_server_utils_posix.h, line 86 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Is this struct being created/destroyed with C++ new/delete or with C malloc/free? If it's going to include a C++ type, we should make sure it's the former.

It's just a pointer to vector, not an actual vector. I should mark that this is an unowned pointer (the owner is actually the grpc_server)

src/core/lib/iomgr/udp_server.cc, line 192 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same question here about new/delete vs malloc/free.

Will mark that the pointer isn't owned by this struct.

src/core/lib/surface/server.h, line 84 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Since these methods are not part of the C-core public API, they can be in C++ idiom, not C idiom. So why not expose the definition of grpc_server and just make these methods of that object?

In fact, I suspect most of the APIs in this file can be nested inside of the grpc_server type.

will do.

src/core/lib/surface/server.h, line 85 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

What's the ownership model of ListenerCallbackInterface? I was expecting this to take a unique_ptr<> and delete the instance when the listener is destroyed.

I'll document this.

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

Previously, markdroth (Mark D. Roth) wrote…

Pretty much everything in this file should be moved to the grpc_core namespace and changed to use C++ idioms (e.g., standalone functions replaced with methods, use C++ naming conventions, etc). Most of this code can probably be nested inside of grpc_server.

Most of this lives only in this file, so I'd still like to keep it in the anonymous namespace like it is right now. But I do like the idea of nesting most under grpc_server.

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

Previously, markdroth (Mark D. Roth) wrote…

Seems a little strange to have a separate struct for this. Why not just make this the same as ListenerCallbackInterface and have the caller pass in a new instance?

I was trying to keep it a pure interface so as not to eliminate the possibility of multiple inheritance but given that this isn't API, I think I can relax that requirement.

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

Previously, markdroth (Mark D. Roth) wrote…

I assume this indicates whether list_position is set? If so, suggest just making list_position an absl::optional<>.

Will do.

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

Previously, markdroth (Mark D. Roth) wrote…

Data members should be private and have a _ suffix.

This is a struct at the current time, so I haven't followed that requirement. Will do when converting to a class.

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

Previously, markdroth (Mark D. Roth) wrote…

Can we make this inherit from InternallyRefCounted<>, so that we can use RefCountedPtr<> to manage the refs?

will do.

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

Previously, markdroth (Mark D. Roth) wrote…

Would it help performance to do channels_.reserve(s->channels.size()) here?

It's only invoked at server shutdown time so I haven't done anything to optimize performance. Seems like an easy addition, though, so I'll do it.

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

Previously, markdroth (Mark D. Roth) wrote…

std::list<> does have to allocate a small amount of memory for each node in the list. Is there a performance impact of this change?

This is the slow-path; by the time pending_ is used, we have bigger problems on hand (insufficient requests outstanding), so I haven't tried to optimize. I'll call this WAI.

vjpai

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

src/core/lib/surface/server.h, line 75 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of having an OnDestroy() method, why not just have the server destroy the listener, and trigger whatever shutdown behavior is needed via the dtor? This would require the destroy_done closure to be set earlier (maybe via the ctor?), but that seems doable.

As I revisited this review: it's nuanced because it's actually the listeners' completion that destroys the server. (The listeners are told to stop listening early, and only after they've all fully completed their shutdown process (which is listener-specific) are they supposed to invoke their destroy_done closure, and only after all those closures have been invoked does the server actually destroy.) I think this should be made more clear by renaming this from OnDestroy to OnStop (which is also parallel with OnStart, so added bonus).

markdroth · 2020-06-02T18:25:26Z

I've just pushed a commit to #23104 that C++-ifies the listener API. It uses Orphanable for listeners, so the server simply orphans the listener when it wants it to shut down.

vjpai · 2020-06-24T09:06:51Z

Restarting this PR after a while, so far have only merged and resolved all conflicts, but will soon address remaining reviewer comments.

vjpai

Reviewable status: 9 of 21 files reviewed, 18 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 52 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Sounds good, I'll do this. It'll probably come through next week since I have some other priority items, but might as well do it right.

Marking this resolved (without any new contributions in this PR)

src/core/lib/iomgr/tcp_server_utils_posix.h, line 86 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

It's just a pointer to vector, not an actual vector. I should mark that this is an unowned pointer (the owner is actually the grpc_server)

Done.

src/core/lib/iomgr/udp_server.cc, line 192 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Will mark that the pointer isn't owned by this struct.

Done.

src/core/lib/surface/server.h, line 75 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

As I revisited this review: it's nuanced because it's actually the listeners' completion that destroys the server. (The listeners are told to stop listening early, and only after they've all fully completed their shutdown process (which is listener-specific) are they supposed to invoke their destroy_done closure, and only after all those closures have been invoked does the server actually destroy.) I think this should be made more clear by renaming this from OnDestroy to OnStop (which is also parallel with OnStart, so added bonus).

Resolved without any work in this PR.

src/core/lib/surface/server.h, line 85 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

I'll document this.

Resolved elsewhere.

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

Previously, vjpai (Vijay Pai) wrote…

Most of this lives only in this file, so I'd still like to keep it in the anonymous namespace like it is right now. But I do like the idea of nesting most under grpc_server.

Resolved elsewhere

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

Previously, vjpai (Vijay Pai) wrote…

I was trying to keep it a pure interface so as not to eliminate the possibility of multiple inheritance but given that this isn't API, I think I can relax that requirement.

Resolved elsewhere

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

Previously, vjpai (Vijay Pai) wrote…

This is the slow-path; by the time pending_ is used, we have bigger problems on hand (insufficient requests outstanding), so I haven't tried to optimize. I'll call this WAI.

Done

test/core/surface/concurrent_connectivity_test.cc, line 97 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same question as elsewhere regarding new/delete vs. malloc/free.

Done via a comment fix (this is only stack-allocated)

test/core/util/test_tcp_server.h, line 32 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same question as elsewhere regarding new/delete vs. malloc/free.

Done via a comment fix (this is only stack-allocated)

vjpai

Reviewable status: 9 of 21 files reviewed, 18 unresolved discussions (waiting on @markdroth and @vjpai)

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

Previously, vjpai (Vijay Pai) wrote…

Will do.

Done.

vjpai

Reviewable status: 9 of 21 files reviewed, 18 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 225 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This would be a bit cleaner if written to avoid ReleasableMutexLock by moving the code that requires the lock into a helper function:

grpc_core::RefCountedPtr<grpc_core::HandshakeManager> CreateHandshakeManager(server_state* state) {
  grpc_core::MutexLock lock(&state->mu);
  if (state->shutdown) return nullptr;
  grpc_resource_user* resource_user =
      grpc_server_get_default_resource_user(state->server);
  if (resource_user != nullptr &&
      !grpc_resource_user_safe_alloc(resource_user,
                                     GRPC_RESOURCE_QUOTA_CHANNEL_SIZE)) {
    gpr_log(
        GPR_ERROR,
        "Memory quota exhausted, rejecting the connection, no handshaking.");
    return nullptr;
  }
  auto handshake_mgr = grpc_core::MakeRefCounted<grpc_core::HandshakeManager>();
  handshake_mgr->AddToPendingMgrList(&state->pending_handshake_mgrs);
  grpc_tcp_server_ref(state->tcp_server);
  return handshake_mgr;
}

Then the code here becomes:

auto handshake_mgr = CreateHandshakeManager(state);
if (handshake_mgr == nullptr) {
  grpc_endpoint_shutdown(tcp, GRPC_ERROR_NONE);
  grpc_endpoint_destroy(tcp);
  gpr_free(acceptor);
  return;
}

Done (comment is on obsoleted code)

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

Previously, markdroth (Mark D. Roth) wrote…

I generally am not a big fan of ReleasableMutexLock; I think in most cases where it is used, the code would be clearer if restructured to use a normal MutexLock. I think this is a good example of that. As an alternative, consider moving the code that requires the lock into a helper method like this:

struct RequestedCallEntry {
  requested_call* rc;
  call_data* calld;  
};

RequestedCallEntry GetNextRequestedCall() {
  grpc_core::MutexLock lock(&server_->mu_call);
  if (pending_.empty()) return {nullptr, nullptr};
  RequestedCallEntry entry;
  entry.rc = reinterpret_cast<requested_call*>(
      requests_per_cq_[request_queue_index].Pop());
  if (entry.rc == nullptr) return {nullptr, nullptr};
  entry.calld = pending_.front();
  pending_.pop_front();
  return entry;
}

Then the loop here becomes the following:

while (true) {
  RequestedCallEntry entry = GetNextRequestedCall();
  if (entry.rc == nullptr) break;
  if (!gpr_atm_full_cas(&entry.calld->state, PENDING, ACTIVATED)) {
    // Zombied Call
    GRPC_CLOSURE_INIT(
        &entry.calld->kill_zombie_closure, kill_zombie,
        grpc_call_stack_element(grpc_call_get_call_stack(entry.calld->call), 0),
        grpc_schedule_on_exec_ctx);
    grpc_core::ExecCtx::Run(DEBUG_LOCATION, &entry.calld->kill_zombie_closure,
                            GRPC_ERROR_NONE);
  } else {
    publish_call(server_, entry.calld, request_queue_index, entry.rc);
  }
}

Done, though I borrowed your technique of using a lambda for this rather than creating a new class function just to emphasize that this is just a helper for this function only.

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

Previously, markdroth (Mark D. Roth) wrote…

Similar comment here: Please see if there's a cleaner way to avoid ReleasableMutexLock.

Done, similar to above.

vjpai

Reviewable status: 9 of 21 files reviewed, 18 unresolved discussions (waiting on @markdroth and @vjpai)

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

Previously, vjpai (Vijay Pai) wrote…

It's only invoked at server shutdown time so I haven't done anything to optimize performance. Seems like an easy addition, though, so I'll do it.

Done.

markdroth

This is moving in the right direction! Comments are mainly minor at this point, but still waiting for remaining C++-ification.

Thanks for doing this!

Reviewed 5 of 6 files at r2, 1 of 3 files at r3, 7 of 7 files at r4.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @markdroth and @vjpai)

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

Previously, vjpai (Vijay Pai) wrote…

Resolved elsewhere

In general, we've been moving things to the grpc_core namespace as we convert them to idiomatic C++. If it's also local to the file, it can be in an anonymous namespace inside of the grpc_core namespace.

src/core/lib/surface/server.cc, line 23 at r4 (raw file):

#include "src/core/lib/surface/server.h"

#include <grpc/support/alloc.h>

These were actually in the right place before this change.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

src/core/lib/surface/server.cc, line 248 at r4 (raw file):

  grpc_call* call;

  gpr_atm state = NOT_STARTED;

Convert to grpc_core::Atomic<>?

src/core/lib/surface/server.cc, line 515 at r4 (raw file):

      /* this was the first queued request: we need to lock and start
         matching calls */
      struct pending_call {

PendingCall

src/core/lib/surface/server.cc, line 578 at r4 (raw file):

    // an empty request queue, it will block until the call is actually
    // added to the pending list.
    struct check_queue_result {

CheckQueueResult

src/core/lib/surface/server.cc, line 584 at r4 (raw file):

    };

    auto check_queues = [this, start_request_queue_index] {

In this particular case, there's not really any benefit to using a lambda here. Instead, I suggest writing it like this:

requested_call* rc = nullptr;
size_t cq_idx;
size_t loop_count;
{
  MutexLock lock(&server_->mu_call);
  for (loop_count = 0; loop_count < requests_per_cq_.size(); loop_count++) {
    cq_idx = (start_request_queue_index + loop_count) % requests_per_cq_.size();
    rc = reinterpret_cast<requested_call*>(requests_per_cq_[cq_idx].Pop());
    if (rc != nullptr) break;
  }
  if (rc == nullptr) {
    gpr_atm_no_barrier_store(&calld->state, PENDING);
    pending_.push_back(calld);
    return;
  }
}
GRPC_STATS_INC_SERVER_CQS_CHECKED(loop_count +
                                  requests_per_cq_.size());
gpr_atm_no_barrier_store(&calld->state, ACTIVATED);
publish_call(server_, calld, cq_idx, rc);

src/core/lib/surface/server.cc, line 1647 at r4 (raw file):

grpc_call_error grpc_server_request_call(
    grpc_server* server, grpc_call** call, grpc_call_details* details,
    grpc_metadata_array* request_metadata,

Why the rename here? This is initial metadata, so that name seemed appropriate.

src/core/lib/surface/server.cc, line 1675 at r4 (raw file):

grpc_call_error grpc_server_request_registered_call(
    grpc_server* server, void* rmp, grpc_call** call, gpr_timespec* deadline,
    grpc_metadata_array* request_metadata, grpc_byte_buffer** optional_payload,

Same question here about the parameter rename.

src/core/lib/surface/server.cc, line 1677 at r4 (raw file):

    grpc_metadata_array* request_metadata, grpc_byte_buffer** optional_payload,
    grpc_completion_queue* cq_bound_to_call,
    grpc_completion_queue* cq_for_notification, void* tag_new) {

And why tag_new here?

test/core/surface/concurrent_connectivity_test.cc, line 96 at r4 (raw file):

// Always stack-allocate or new server_thread_args; never use gpr_malloc since
// this contains C++ objects.
struct server_thread_args {

ServerThreadArgs

test/core/util/test_tcp_server.h, line 29 at r4 (raw file):

// test_tcp_server should be stack-allocated or new'ed, never gpr_malloc'ed
// since it contains C++ objects.
struct test_tcp_server {

TestTcpServer

vjpai

Oops, I was going to ping you with a PTAL at some point, but thanks for the early reviews! I'll get on these before asking for that PTAL.

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

vjpai

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

src/core/lib/surface/server.cc, line 23 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

These were actually in the right place before this change.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

I'm guessing that I used g4 fix here and got a different outcome. I'll revert this change.

src/core/lib/surface/server.cc, line 1647 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Why the rename here? This is initial metadata, so that name seemed appropriate.

I agree that that was a better name, but this is the name we've been using in grpc/grpc.h since forever apparently, and I got a linter complaint about argument name mismatch which maybe we've been ignoring for a while.

src/core/lib/surface/server.cc, line 1675 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Same question here about the parameter rename.

Same story but worse in this case since there's still a linter complaint after the fix about rmp (which I didn't rename to the grpc.h version since that's a struct name in this file)

src/core/lib/surface/server.cc, line 1677 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

And why tag_new here?

Same reason. (To be clear, I hadn't decided yet if I was going to let these changes stick and was thinking of reverting them to stick to the single-purpose CL rule)

test/core/surface/concurrent_connectivity_test.cc, line 96 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

ServerThreadArgs

Do we have a rule on when to follow C-style vs C++ style on struct's ? Is it that surface API structs are the only ones that should keep snake-case names longer-term?

markdroth

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

test/core/surface/concurrent_connectivity_test.cc, line 96 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Do we have a rule on when to follow C-style vs C++ style on struct's ? Is it that surface API structs are the only ones that should keep snake-case names longer-term?

Yeah, I think that's the right rule. IMHO, anything internal that has been converted to idiomatic C++ should follow C++ naming conventions.

vjpai

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

src/core/lib/surface/server.h, line 84 at r1 (raw file):

Previously, vjpai (Vijay Pai) wrote…

will do.

As discussed OOB, I won't be doing this at this point since the decision of moving grpc_server to grpc_core::Server etc will go to a different PR.

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

Previously, markdroth (Mark D. Roth) wrote…

In general, we've been moving things to the grpc_core namespace as we convert them to idiomatic C++. If it's also local to the file, it can be in an anonymous namespace inside of the grpc_core namespace.

Done.

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

Previously, vjpai (Vijay Pai) wrote…

This is a struct at the current time, so I haven't followed that requirement. Will do when converting to a class.

Punting class conversion to a later PR by you....

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

Previously, vjpai (Vijay Pai) wrote…

will do.

I feel that's part of class conversion (since a struct should just passively hold data), so outside this PR.

src/core/lib/surface/server.cc, line 248 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Convert to grpc_core::Atomic<>?

Done.

src/core/lib/surface/server.cc, line 515 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

PendingCall

Done.

src/core/lib/surface/server.cc, line 578 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

CheckQueueResult

Removed altogether following your other comment below.

src/core/lib/surface/server.cc, line 584 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

In this particular case, there's not really any benefit to using a lambda here. Instead, I suggest writing it like this:

requested_call* rc = nullptr;
size_t cq_idx;
size_t loop_count;
{
  MutexLock lock(&server_->mu_call);
  for (loop_count = 0; loop_count < requests_per_cq_.size(); loop_count++) {
    cq_idx = (start_request_queue_index + loop_count) % requests_per_cq_.size();
    rc = reinterpret_cast<requested_call*>(requests_per_cq_[cq_idx].Pop());
    if (rc != nullptr) break;
  }
  if (rc == nullptr) {
    gpr_atm_no_barrier_store(&calld->state, PENDING);
    pending_.push_back(calld);
    return;
  }
}
GRPC_STATS_INC_SERVER_CQS_CHECKED(loop_count +
                                  requests_per_cq_.size());
gpr_atm_no_barrier_store(&calld->state, ACTIVATED);
publish_call(server_, calld, cq_idx, rc);

Done.

src/core/lib/surface/server.cc, line 1647 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

I agree that that was a better name, but this is the name we've been using in grpc/grpc.h since forever apparently, and I got a linter complaint about argument name mismatch which maybe we've been ignoring for a while.

I'll unwind this and bring back the linter warning so we think about doing something about it in the future.

src/core/lib/surface/server.cc, line 1675 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Same story but worse in this case since there's still a linter complaint after the fix about rmp (which I didn't rename to the grpc.h version since that's a struct name in this file)

Reverting this change too.

src/core/lib/surface/server.cc, line 1677 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Same reason. (To be clear, I hadn't decided yet if I was going to let these changes stick and was thinking of reverting them to stick to the single-purpose CL rule)

I'd like to keep this one as it's not confusing (this is a tag for the new call, after all)

test/core/surface/concurrent_connectivity_test.cc, line 96 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Yeah, I think that's the right rule. IMHO, anything internal that has been converted to idiomatic C++ should follow C++ naming conventions.

Done.

test/core/util/test_tcp_server.h, line 29 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

TestTcpServer

Done.

vjpai

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

test/core/util/test_tcp_server.h, line 29 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Done.

Actually, reverting that one because it has too much collateral damage.

markdroth

Thanks for doing this!

The only substantive remaining issue is that channel_data needs C++-style initialization and destruction. The other comments are mostly cosmetic.

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

Reviewed 1 of 7 files at r4, 10 of 10 files at r5.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @vjpai)

src/core/lib/iomgr/tcp_server_windows.cc, line 29 at r5 (raw file):

#include <inttypes.h>
#include <io.h>
#include <vector>

As per style guide, there's supposed to be a blank line before this include.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

src/core/lib/surface/server.cc, line 23 at r4 (raw file):

Previously, vjpai (Vijay Pai) wrote…

I'm guessing that I used g4 fix here and got a different outcome. I'll revert this change.

This still needs to be fixed.

src/core/lib/surface/server.cc, line 1204 at r5 (raw file):

  GPR_ASSERT(args->is_first);
  GPR_ASSERT(!args->is_last);
  chand->server = nullptr;

channel_data now has C++ data members, so it should really be initialized and destroyed as a C++ object.

Please move the initialization and destruction into a ctor and dtor, construct it here with placement-new, and explicitly call the dtor in server_destroy_channel_elem().

src/core/lib/surface/server.cc, line 1433 at r5 (raw file):

  }
  if (server->unregistered_request_matcher == nullptr) {
    server->unregistered_request_matcher.reset(

server->unregistered_request_matcher =
    absl::make_unique<grpc_core::RealRequestMatcher>(server);

src/core/lib/surface/server.cc, line 1439 at r5 (raw file):

       server->registered_methods) {
    if (rm->matcher == nullptr) {
      rm->matcher.reset(new grpc_core::RealRequestMatcher(server));

rm->matcher = absl::make_unique<grpc_core::RealRequestMatcher>(server);

vjpai

Thanks for the review! I think I've addressed the remaining requirements.

Reviewable status: 19 of 21 files reviewed, 7 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/lib/iomgr/tcp_server_windows.cc, line 29 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

As per style guide, there's supposed to be a blank line before this include.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

Done.

src/core/lib/surface/server.cc, line 23 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This still needs to be fixed.

I think it's done now, I hope....

src/core/lib/surface/server.cc, line 1204 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

channel_data now has C++ data members, so it should really be initialized and destroyed as a C++ object.

Please move the initialization and destruction into a ctor and dtor, construct it here with placement-new, and explicitly call the dtor in server_destroy_channel_elem().

Done.

src/core/lib/surface/server.cc, line 1433 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

server->unregistered_request_matcher =
    absl::make_unique<grpc_core::RealRequestMatcher>(server);

Done. I sat on this PR so long that we started depending on different libraries in the meanwhile....

src/core/lib/surface/server.cc, line 1439 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

rm->matcher = absl::make_unique<grpc_core::RealRequestMatcher>(server);

Done.

markdroth

Looks great! Feel free to merge after addressing the one remaining comment.

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @vjpai)

src/core/lib/surface/server.cc, line 137 at r6 (raw file):

  grpc_server* server = nullptr;
  grpc_channel* channel;

Shouldn't this also be initialized to nullptr?

vjpai

Reviewable status: 20 of 21 files reviewed, 3 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/lib/surface/server.cc, line 137 at r6 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Shouldn't this also be initialized to nullptr?

Actually, neither server nor channel needed an initialization because they both get set up in server_setup_transport. I erred on the side of caution with server since the destructor uses it (so an object that is just constructed and then destructed would have a problem), but channel cannot be used by anything until server_setup_transport.

vjpai

Reviewable status: 20 of 21 files reviewed, 3 unresolved discussions (waiting on @markdroth and @vjpai)

src/core/lib/surface/server.cc, line 137 at r6 (raw file):

Previously, vjpai (Vijay Pai) wrote…

Actually, neither server nor channel needed an initialization because they both get set up in server_setup_transport. I erred on the side of caution with server since the destructor uses it (so an object that is just constructed and then destructed would have a problem), but channel cannot be used by anything until server_setup_transport.

Alternate phrasing: channel is meaningless if server is nullptr, and will get set to something meaningful when server is changed from nullptr.

vjpai · 2020-07-20T10:16:03Z

Squashing commits, thanks for all the reviews!

Fix plugin initializers Previous code not threadsafe nor idiomatic Fix server_builder_plugin_test Bump version to v1.30.0-pre1 Regenerate projects Merge pull request grpc#23155 from srini100/v1.30.x bump v1.30.x branch to v1.30.0-pre1 Changes needed to enable TLS 1.3. Cleanup |ssl_bytes_remaining| per Yang's comments. Check unused bytes size is smaller than received bytes size, and fix Python format in _server_ssl_cert_config_test.py Revert "Check unused bytes size is smaller than received bytes size, and fix Python format in _server_ssl_cert_config_test.py" This reverts commit 7d0ae9c. Add check that unused bytes size is at most received bytes size. Add back source-only ruby packages for 1.30.x Fix memory leak, per Yang's comment. Initial working implementation Avoid collisions in cs files generated by Grpc.Tools Create generated directories Updated unit tests Reverted GetDepFilenameForProto Otherwise it creates conflicts with non-standard $(Protobuf_DepFilesPath) Improved folder generation Use same logic in csharp as in cpp Removed unused usings Reverted whitespace changes Readded InteropServices Needed #if NETCORE Bring closer to master Add support for Protobuf_ProtoRoot Fix false positive in grpc_is_epollexclusive_available on ESXi Improve third_party readme Fix up aio interop client Allow the user to specify grpclb config Merge remote-tracking branch 'upstream/master' into matt-tls13 Merge pull request grpc#23206 from ericgribkoff/dynamic_server_health Add gentle failover test Merge pull request grpc#23248 from ericgribkoff/use_grpc_health_check Use grpc health checks for PHP and Ruby xDS tests Run clang-format on is_epollexclusive_available.cc Merge pull request grpc#23250 from ericgribkoff/backport_gentle_failover Backport gentle failover Add TLS 1.2 testing. Merge remote-tracking branch 'upstream/master' into matt-tls13 Add #ifdef around use of SSL_ctx_min/max_proto_version Add include of tls1.h for OpenSSL. I is a reserved identifier Update links to grpc.io guides in header files Fix the grpcpp.h formatting. Merge pull request grpc#23186 from apolcyn/upmerge_source_only_to_1_30x Add back source-only ruby packages for 1.30.x Trust google-auth library will handle their dependency properly Install rsa==4.0 to avoid the egg cache race issue Add tls1.h header regardless of what SSL library is used. Bump version to 1.30.0 Regenerate projects Add #if defined(TLS1_3_VERSION). Merge pull request grpc#23264 from lidizheng/v1.30.x-patch Backport: Trust google-auth library will handle their dependency properly Merge pull request grpc#23266 from srini100/v1.30.x Bump to v1.30.0 Fix whitespaces Release dispatch queue on CFStreamHandle destroy Address David's comments. Move ServerPosix from ::grpc_impl to ::grpc Revert https://github.com/grpc/grpc/pull/18372/files Avoid attribute error in __del__ from _ChannelCallState Guard _ChannelCallState.__del__ from cygrpc being deallocated early Merge pull request grpc#23291 from lidizheng/v1.30.x-patch-2 Backport: Avoid attribute error in __del__ of _ChannelCallState Revert "Revert "Move ServerBuilder back to ::grpc from ::grpc_impl"" fix from sanitize.sh Merge pull request grpc#23303 from karthikravis/revert-23294-revert-23182-server-builder fix from sanitize.sh Merge branch 'master' into server-posix Move create_channel_posix from ::grpc_impl to ::grpc Revert grpc#18373 Merge remote-tracking branch 'upstream/master' into matt-tls13 Move create_channel and credentials from ::grpc_impl to ::grpc Reverts: grpc#18374 and grpc#18444 Credentials and create_channel are very closely intertwined, so it is easier to migrate them together. Make build and formatting fixes Add tests for unused bytes. Formatting fixes Implement gce_channel_credentials in core Fix formatting and build files Regenerate projects Initial implementation of gce_channel_creds Format Address Jiangtao's comments. Change name to compute_engine_channel_credentials Let the wrapped language create the composite credential Remove use of TLSv1_2_method. Expose gce tenancy as a function Regenerate projects Yapf :( Remove inline Stop double caching Add test Remove weird, seemingly pointless variable Clang format Main Makefile should require protobuf 3.12+ now Remove platform-specific assertion Buildifier Fix use-after-free in ruby call creds Lower log level to INFO from ERROR from keepalive watchdog firing Mark core API as experimental Use a gerund for a function name Simplify reference count juggling Clang format Merge pull request grpc#19756 from apolcyn/fix_ruby_md Fix auth plugin context use-after-free in ruby Make sure call creds user callbacks can't kill the ruby call credentials thread Update license date; fix format and typo Added missing deps_linkage Regenerated projects Fix typo Merge pull request grpc#23221 from chalin/chalin-grpc-io-link-fixes-200616 Update links to grpc.io guides in header files Merge pull request grpc#23335 from veblush/deps-linkage Added missing `deps_linkage` property Added missing deps_linkage Generated projects Fix gRPC-C++.podspec file Merge branch 'master' into revert-23294-revert-23182-server-builder Merge branch 'revert-23294-revert-23182-server-builder' of https://github.com/grpc/grpc into revert-23294-revert-23182-server-builder Merge pull request grpc#23324 from stanley-cheung/makefile-min-protobuf-version Main Makefile should require protobuf 3.12+ now Merge pull request grpc#23337 from CorralPeltzer/python-doc-typo Fix typo in docstring Merge branch 'master' into create-channel Fix credentials test. Address David's comments. Formatting fixes Suppress exceptions from the __del__ of channel object Merge pull request grpc#23304 from karthikravis/server-posix Move server posix from ::grpc_impl to ::grpc Merge pull request grpc#23305 from karthikravis/channel-posix Move create_channel_posix from ::grpc_impl to ::grpc Add back ciphersuites in ssl_utils.cc Merge pull request grpc#23345 from karthikravis/revert-23294-revert-23182-server-builder Revert "Revert "Move ServerBuilder back to ::grpc from ::grpc_impl"" Fix fetch oauth2 test Fix formatting issue Replaced grpc::string with std::string Merge pull request grpc#23350 from veblush/grpc-string2 Replaced grpc::string with std::string Removed grpc_artifact_protoc/Dockerfile Refactored AR in makefile Remove things_queued_ever counter from callback CQ Refactored AR in makefile Add abseil compiler options unify Grpc.Tools projects with other csproj projects Add missing include. Merge pull request grpc#23331 from yashykt/keepalivewatchdoginfolog Lower log level to INFO from ERROR from keepalive watchdog firing Fix connect deadline issue in settings_timeout_test Fix possible deadlock in RemoveExternalConnectivityWatcher Objc support PB runtime import override Fix possible deadlock in RemoveExternalConnectivityWatcher Merge branch 'master' into create-channel Merge pull request grpc#23354 from grpc/vjpai-patch-1 Remove things_queued_ever counter from callback CQ Merge pull request grpc#23364 from yashykt/settingstimeouttest Fix connect deadline issue in settings_timeout_test Removed GRPC_USE_ABSL Merge pull request grpc#23237 from grpc/jtattermusch-patch-1 Improve third_party readme (checklist for adding dependencies) Merge pull request grpc#22869 from Falco20019/grpc-fix-collisions Avoid collisions in cs files generated by Grpc.Tools update submodule boringssl-with-bazel with origin/master-with-bazel update boringssl dependency to master-with-bazel commit SHA Merge pull request grpc#23360 from markdroth/llvm_build_fix Add missing include. Merge pull request grpc#23165 from matthewstevenson88/matt-tls13 Enable TLS 1.3 in the C-core and all wrapped languages. Fix message_allocator_e2e_test Merge pull request grpc#23356 from veblush/ar-makefile Refactored AR in makefile Merge pull request grpc#23357 from veblush/absl-cppflags Add abseil compiler options Merge pull request grpc#23352 from veblush/protoc-docker Removed grpc_artifact_protoc/Dockerfile Merge pull request grpc#23298 from yulin-liang/prefix_import_path Objc support PB runtime import override Merge pull request grpc#23371 from veblush/del-GRPC_USE_ABSL Removed GRPC_USE_ABSL Merge pull request grpc#23376 from yang-g/allocator_test Fix message_allocator_e2e_test Added call to grpc::testing::TestEnvironment in tests Merge pull request grpc#23367 from yashykt/130fixws Fix possible deadlock in RemoveExternalConnectivityWatcher Merge pull request grpc#23378 from veblush/harden-tests Added call to grpc::testing::TestEnvironment in tests Added call to grpc::testing::TestEnvironment in more tests Merge branch 'master' into create-channel Merge pull request grpc#23379 from veblush/harden-tests2 Added call to grpc::testing::TestEnvironment in more tests Merge pull request grpc#23276 from grpc/cfstream-queue-release Release dispatch queue on CFStreamHandle destroy Add comment Merge pull request grpc#23333 from apolcyn/bad_md_drops_call_creds_thread Make sure call creds user callbacks can't kill the ruby call credentials thread Merge pull request grpc#23365 from yashykt/deadlockws Fix possible deadlock in RemoveExternalConnectivityWatcher Passed the repo manager to veblush Merge pull request grpc#23392 from veblush/to-veblush Passed the repo manager to veblush Increase size of a test to match internal guidance Pin isort to fix sanity test breakage New Matchers Implementation - All except for Regex (which will be submitted with RE2 import PR) Merge pull request grpc#22813 from mehrdada/fix-static-initialization-crap Fix channelz plugin initializer Merge pull request grpc#23395 from gnossen/lint_fix Pin isort to fix sanity test breakage Merge remote-tracking branch 'upstream/master' into matcher_new Merge pull request grpc#22951 from donnadionne/matcher_new New Matchers Add missing string include Get new core API design building Fix use-after-free in ruby call creds Add Python wrapper Pin isort to fix sanity test breakage Added TCP_USER_TIMEOUT auto-detection Merge pull request grpc#23393 from grpc/vjpai-patch-1 Increase size of a test to match internal guidance Merge pull request grpc#23400 from apolcyn/backport_sanity_check_fix Backport pylint sanity check fix to 1.30.x Bump 1.30.x branch to 1.30.1 Fix inproc transport bugs on client WritesDone or Read after status Don't drop message on WriteAndFinish with non-ok status regenerate projects generate boringssl prefix headers fix nits in third_party/README.md Increment podspec version regenerate projects Update README.md Add TLS 1.2 and 1.3 specific tests to h2_tls fixture. Merge pull request grpc#23380 from vjpai/inproc_fix Fix inproc transport bug on client WritesDone and server send message after status Merge pull request grpc#23409 from grpc/jtattermusch-patch-2 fix nits in third_party/README.md Merge pull request grpc#23386 from Kernald/fix-string-includes Add missing string include Merge pull request grpc#23408 from grpc/vjpai-patch-1 Don't drop message on WriteAndFinish with non-ok status Merge pull request grpc#23401 from veblush/tcp_user_timeout Added TCP_USER_TIMEOUT auto-detection Add gRPC-specific docker images for Ruby build Make pluing embed zlib Allows poller to bound to ephemeral loops in multiple threads Address comments Add type annotation in comments Merge pull request grpc#23399 from apolcyn/backport_auth_plugin_ruby_fix Backport fix for use-after-free in ruby auth plugin Replace most uses of gpr_asprintf() with absl calls. Merge pull request grpc#23387 from veblush/protoc-docker Make plugins embed zlib Merge pull request grpc#23377 from lidizheng/aio-multi-loop [Aio] Allows poller to bound to ephemeral loops in multiple threads Fix FailHijackedRecvMessage for generic async APIs Update grpclb with optional field "name" Add test for the new config field Address comments Add support for xDS regex matchers. Merge pull request grpc#23351 from lidizheng/silence-del Suppress exceptions from the __del__ of channel object Make sure that some ops don't start concurrently with Finish Merge pull request grpc#23405 from apolcyn/bump_version Bump 1.30.x branch to 1.30.1 Merge pull request grpc#22987 from donnadionne/new Add support for xDS regex matchers. Merge pull request grpc#23157 from markdroth/gpr_asprintf Replace most uses of gpr_asprintf() with absl calls. Store ref to the ExternalConnectivityWatcher in external_watchers_ map. Add comment Fix ruby 2.7 keyword arguments deprecation Similar to grpc#22915 Properly count messages sent Merge pull request grpc#23413 from matthewstevenson88/tls13-h2-tls-fixture Add TLS 1.2 and 1.3 specific tests to h2_tls fixture. Stop trying to handle SIGSEGV Make request path more easily visible to LB policies. Make warning more dire Merge pull request grpc#23416 from vjpai/ccet Make sure that some test ops don't start concurrently with Finish Merge pull request grpc#23421 from gnossen/test_runner_segfault Stop trying to handle SIGSEGV in Test Runner Merge pull request grpc#23258 from tweej/I_is_reserved I is a reserved identifier Merge pull request grpc#23417 from markdroth/lb_policy_path Make request path more easily visible to LB policies. Reviewer comments WIP EM-agnostic callback completion queue Fix TCP_USER_TIMEOUT definition Address comments Another comment Merge pull request grpc#23415 from yashykt/genericfailsend Fix FailHijackedRecvMessage for generic async APIs Merge pull request grpc#23425 from veblush/fix-TCP_USER_TIMEOUT Fix TCP_USER_TIMEOUT definition Fix data race in RpcWithEarlyFreeRequest by using atomics Fix data race in RpcWithEarlyFreeRequest by using atomics Merge pull request grpc#23358 from jtattermusch/cleanup_grpc_tools unify Grpc.Tools projects with other csproj projects Merge pull request grpc#23410 from jtattermusch/upgrade_boringssl_submodule Upgrade boringssl submodule to latest master-with-bazel add Grpc.Tools test for generating for .proto with the same name Merge pull request grpc#23427 from vjpai/message_allocator_race Fix data race in RpcWithEarlyFreeRequest by using atomics Merge pull request grpc#23361 from vjpai/em_agnostic_core_callback_cq EM-agnostic core callback CQ Remove unnecessary RPC Remove references to skipping callback API tests Reviewer comments Merge pull request grpc#22345 from muxi/grpclb-name-config Update grpclb configuration with field "service_name" Add sunny day core API path Merge pull request grpc#23419 from miyucy/patch-1 Fix ruby 2.7 keyword arguments deprecation Update the hint for install Python header to depend on version Stop using mutex for non-polling engine marker Make sure >3 Python version triggers a failure Merge pull request grpc#23418 from yashykt/dataraceexternalcancel Store ref to the ExternalConnectivityWatcher in external_watchers_ map. Merge pull request grpc#23430 from vjpai/remove_do_not_test Remove references to skipping callback API tests Remove unused variable (fixes builder error) Merge pull request grpc#23435 from vjpai/fix_iomgr_non_polling Fix-forward: Stop using mutex for non-polling engine marker Add test Clean up Reuse mdelem Merge remote-tracking branch 'origin/master' into python_google_default_creds Merge pull request grpc#23440 from grpc/vjpai-patch-1 Remove unused variable (fixes builder error) Merge pull request grpc#23375 from jtattermusch/csharp_proto_collision_test Add Grpc.Tools test for generating for .proto with the same name Added change_repo_manager.sh Changed a repo manager to karthikrs add comments for constants Fix initialization order Complete synchronously Accomodate PHP examples/README rework - Fix link to "Intro to gRPC" (formerly "Overview") - Don't list wrapped languages twice (once in intro paragraph, then in subdirectory list); move subdirectory links to the start of the page. - Sort subdirectory list - Link to grpc.io "Supported Languages" page for the remaining languages Merge pull request grpc#23446 from yang-g/init_order Fix initialization order Merge pull request grpc#23445 from HannahShiSFB/add-comments4constants PHP: add comments for constants make port server python3 compatible fix livelock in concurrent_connectivity_test on windows update bigquery partition expiration Update grpc_release_schedule.md Remove xds-experimental URI scheme. Merge pull request grpc#23444 from veblush/to-karthikrs Changed a repo manager to karthikrs Merge pull request grpc#22201 from chrisse74/memoryLeakBuildAndStart fix memory leak of grpc_resource_user_quota Merge pull request grpc#23437 from lidizheng/update-support-message Update the hint for install Python header to depend on version Merge branch 'master' into create-channel Fix build error Further reduce the spam log by doing nothing if GC has error Reorganize header Change signature Merge pull request grpc#23463 from markdroth/xds_experimental Remove xds-experimental URI scheme. Clean up Format Typo Merge remote-tracking branch 'origin/master' into python_google_default_creds Regenerate projects Merge pull request grpc#23447 from chalin/chalin-ex-readme-200710 examples/README rework Add XdsConfigSelector and populate call attribute containing the deadline. Fix secure test Fix formatting Merge pull request grpc#23467 from lidizheng/dealloc-message Further reduce the spam log by doing nothing if GC has error Update by review Move AuthMetaDataProcessor from ::grpc_impl to ::grpc Reverts: https://github.com/grpc/grpc/pull/18436/files Move GenericStub from ::grpc_impl to ::grpc Reverts: grpc#18434 Move LoadReportingServiceServerBuilderOption from ::grpc_impl to ::grpc Reverts: https://github.com/grpc/grpc/pull/18419/files Move ProtoServerReflectionPlugin from grpc_impl to grpc namespace Reverts: grpc#18600 Merge pull request grpc#23460 from markdroth/xds_config_selector Add XdsConfigSelector and populate call attribute containing the deadline. Fix tests Fix formatting Defer allocation of error object Add to Python documentation Support asyncio interop client Call out unexpected behavior adjust comment Merge pull request grpc#23462 from grpc/jtattermusch-patch-3 Update grpc_release_schedule.md Merge pull request grpc#23457 from jtattermusch/livelock_concurrent_connectivity_test fix potential livelock in global_subchannel_pool.cc bytes(string, encoding) is not supported in python2 results in error "TypeError: str() takes at most 1 argument (2 given)" Merge pull request grpc#23458 from jtattermusch/bigquery_partition_expirations Update bigquery partition expiration (to 365 days) Merge pull request grpc#23455 from jtattermusch/port_server_python3 Make port server python3 compatible Change repo manager to donnadionne This will be merged on 7/20/2020. Merge pull request grpc#23343 from veblush/v1.30.x-ruby [v1.30.x] Backport grpc#23335 (Added missing `deps_linkage` property) Updating C++ interop client and server to do path matching and header matching tests. Merge pull request grpc#23464 from donnadionne/new Updating C++ interop client and server Makefile: stop supporting make install remove rpath trick regenerate projects move routeguide c++ distribtest under cmake_pkgconfig fix C++ interop test build fix PHP interop tests build update PHP readme Merge pull request grpc#23412 from jtattermusch/makefile_remove_installation Simplify makefile: Get rid of "install" rules with pure make, recommend cmake and bazel instead. Include the target name in c-ares and native DNS summary error messages Adding Fake headers for header matching. Construct/destruct ExecCtx between init/shutdown only Add --verbose to dart pub get command Merge pull request grpc#23494 from donnadionne/ga Adding Fake headers for header matching. Concatenate duplicate header keys for matching. Merge pull request grpc#23495 from vjpai/fix_tls Construct/destruct ExecCtx between init/shutdown only Fixing call to absl::StrSplit to deal with empty string case. Merge pull request grpc#23499 from donnadionne/new Fixing call to absl::StrSplit to deal with empty string case. Merge pull request grpc#23496 from stanley-cheung/dart-debug Add --verbose to dart pub get command fix concurrent file read flake Avoid unused param warning Merge pull request grpc#23506 from grpc/vjpai-patch-2 Avoid unused param warning Merge pull request grpc#23493 from apolcyn/error_messages Include the target name in top-level DNS error messages Merge pull request grpc#23468 from karthikravis/auth_metadata Move AuthMetaDataProcessor from ::grpc_impl to ::grpc Merge pull request grpc#23469 from karthikravis/generic-stub Move GenericStub from ::grpc_impl to ::grpc Make more fixes for moving GenericStub from ::grpc_impl to ::grpc Merge pull request grpc#23512 from karthikravis/generic-stub Make more fixes for moving GenericStub from ::grpc_impl to ::grpc Merge pull request grpc#23470 from karthikravis/load-reporting Move LoadReportingServiceServerBuilderOption from ::grpc_impl to ::grpc Fix ruby protoc plugin when message is in another package Ruby protoc plugin fix: fix test breakage Merge pull request grpc#23471 from karthikravis/proto-server Move ProtoServerReflectionPlugin from grpc_impl to grpc namespace Merge pull request grpc#22589 from veblush/ruby-docker Ruby docker for gRPC Manylinux2010-based Ruby images Remove proto_server_reflection_plugin_impl.h from build_autogenerated.yaml Fixing unsafe std::move. Merge pull request grpc#23501 from stanley-cheung/fix-ruby-plugin Fix ruby protoc plugin when message is in another package Fix ruby protoc plugin when message is in another package Merge pull request grpc#23516 from karthikravis/fix-1 Remove proto_server_reflection_plugin_impl.h from build_autogenerated.yaml Merge pull request grpc#23517 from donnadionne/new Fixing unsafe std::move. Merge pull request grpc#23519 from stanley-cheung/ruby-protoc-plugin-fix-backport Backport grpc#23501: Fix ruby protoc plugin when message is in another package Bump version to 1.30.2 Avoid unused param warnings in c_ares directory Merge pull request grpc#23504 from jtattermusch/unflake_verify_peer_options Do not trigger grpc_load_file() flake in verify_peer_options_test Merge pull request grpc#23520 from stanley-cheung/bump-version-1-30-2 Bump version to 1.30.2 Merge pull request grpc#23505 from grpc/vjpai-patch-1 Remove some unused parameter names Merge pull request grpc#23491 from markdroth/xds_header_combining Concatenate duplicate header keys for matching. Revert "Merge pull request grpc#21941 from markdroth/xds_logging" This reverts commit 37d5d93, reversing changes made to 0e8190a. Fix PHP build by adding re2 dependencies Merge pull request grpc#23344 from veblush/ruby-docker-ml Ruby docker image based on manylinux2010 Upgrade BoringSSL to latest on master-with-bazel branch Uses sources.json uses sources.json update rules remove sha256 update sha256 update one more place Merge pull request grpc#23524 from markdroth/xds_logging_revert Revert "Merge pull request grpc#21941 from markdroth/xds_logging" Merge pull request grpc#23526 from stanley-cheung/fix-php-build Fix PHP build by adding re2 dependencies Fix php build from source Revert "Adding Fake headers for header matching." This reverts commit 8c013bf. Fix concatenated_value Fix formatting issue Merge pull request grpc#23538 from karthikravis/fix-xds-breakage Revert "Adding Fake headers for header matching." Update PHP artifact test generate api reference documents by doxygen Added grpc::testing::TestEnvironment Merge pull request grpc#23531 from stanley-cheung/fix-php-build-from-source Fix php build from source Updated comments C++ify core server Merge remote-tracking branch 'upstream/v1.30.x' into HEAD Merge pull request grpc#23236 from tpetkov-VMW/fix-grpc_is_epollexclusive_available Fix false positive in grpc_is_epollexclusive_available on ESXi Remove env var protection of new xds routing code. increase timeout for PHP distribtests Merge pull request grpc#23540 from stanley-cheung/php-artifact-test Update PHP artifact test Merge pull request grpc#23545 from veblush/test-shutdown Added call to grpc::testing::TestEnvironment in tests Merge pull request grpc#23476 from karthikravis/change-owner Change repo manager to donnadionne revert gen_build_yaml.py generate boringssl prefix headers Merge pull request grpc#23548 from jtattermusch/upmerge_1_30_x_to_master Upmerge 1.30.x branch into master Second regeneration update grpc-core podspec template regenerate project Merge pull request grpc#23537 from markdroth/xds_routing_env Remove env var protection of new xds routing code. Protect access Fix docstring Implement fake and ignored headers. Take care to ensure we use fake headers in testing so we don't conflict with headers used by census. Decouple checking tenancy from checking for metadata server Provide an objc-generator option to import system files in a local way Merge pull request grpc#23550 from jtattermusch/php_distrib_15min increase timeout for PHP distribtests Merge pull request grpc#23539 from donnadionne/fix Adding Fake headers and ignored headers for header matching Change xDS ConfigSelector not to pass the timeout header value to the LB policy. Merge pull request grpc#23475 from yulin-liang/local_import_prefix Objc support grpc files import override Initialize the grpc_local_import variable to false. Include re2 in source wheel && solve the missing --all complain Stop checking g_is_on_gce in security connector Merge pull request grpc#23555 from yulin-liang/local_import_prefix Initialize the grpc_local_import variable to false. Log the peer address of grpc_cli CallMethod RPCs to stderr Explicitly fake status if cancelling stream Drop min thread count of threads doing Next to 1 Merge pull request grpc#23255 from HannahShiSFB/api-doxygen-step2 PHP: generate api reference documents by doxygen Merge pull request grpc#23534 from markdroth/xds_fake_header_revert Change xDS ConfigSelector not to pass the timeout header value to the LB policy. stop stripping .hpp files from python source packages Merge pull request grpc#23562 from yang-g/one_core Drop min thread count of threads doing Next to 1 Merge pull request grpc#23528 from emkornfield/upgrade_boring Upgrade BoringSSL to latest on master-with-bazel branch Merge pull request grpc#23552 from lidizheng/re2-source-wheel Include re2 in source wheel && solve the missing --all complain Merge pull request grpc#23565 from jtattermusch/python_dev_stop_excluding_hpp Stop stripping .hpp files from python source packages avoid destroy channel more than once Don't check tenancy if credentials specified Use RAII lock guard Support tuple and aio.Metadata interaction Make pytype happy Only >=3.7 allows annotate oneself as return type Metadata value has to be string or bytes Remove MAX_EPOLL_EVENTS_HANDLED_EACH_POLL_CALL to ensure timely processing of events Remove unused variable Add logs to a flakey ruby test that can help to diagnose issue Add comment Merge pull request grpc#23556 from lidizheng/fix-gapic-tests [Aio] Support tuple and aio.Metadata interaction Merge pull request grpc#23535 from apolcyn/repro_epollex_issue_minimal Remove MAX_EPOLL_EVENTS_HANDLED_EACH_POLL_CALL to ensure timely processing of events Output grpc-dotnet interop apps to specific directories Merge pull request grpc#23570 from apolcyn/fix_ruby_flake Add logs to a flakey ruby test that can help to diagnose issue Also fake status in grpc_chttp2_mark_stream_closed if already closed but there is an error Make ABSL header block consistent with other code. Merge pull request grpc#23558 from yashykt/cancelstreamdeadline Explicitly fake the status for when we cancel the streams Fix Mac and Windows Merge pull request grpc#22757 from vjpai/core_server_cxx C++ify grpc_server Fix a typo Convert multi-line comments to single line comments in chttp2_transport.cc Merge branch 'master' into chttp2transportcomment add g-stands-for for the next release update version to 1.32.0-dev Merge pull request grpc#23523 from JamesNK/jamesnk/interoptests-remove-version Output grpc-dotnet interop apps to specific directories regenerate projects Merge pull request grpc#23203 from gnossen/python_google_default_creds Implement compute_engine_channel_credentials in Python Clang format Plumb absl::Status through connectivity state notifiers Merge pull request grpc#23480 from yashykt/abslstatusconn Plumb absl::Status through connectivity state notifiers Merge branch 'master' into chttp2transportcomment Flag protect new logs Merge pull request grpc#23576 from jtattermusch/bump_master_1_32 update master version to 1.32.0-dev Merge branch 'master' into create-channel Consistency check core_version and version Fixes to match shellcheck Merge pull request grpc#23578 from vjpai/core_check Add a consistency check on version and core_version Merge pull request grpc#23571 from vjpai/typos Typo fix xDS v3 support Merge pull request grpc#23557 from apolcyn/display_peer_address Log the peer address of grpc_cli CallMethod RPCs to stderr fixed merge issues Merge pull request grpc#23281 from markdroth/xds_v3 xDS v3 support Make fixes to cronet_credentials.h Fixing Ruby 1.31.0.pre1 compilation failure with source-only package missing re2 from build_handwritten.yaml and generated grpc.gemspec Merge pull request grpc#23599 from donnadionne/ruby Fixing Ruby compilation failure with source-only package Merge pull request grpc#23308 from karthikravis/create-channel Move create_channel and credentials from ::grpc_impl to ::grpc Disable TLS 1.3 in the C-core. Revert "Move create_channel and credentials from ::grpc_impl to ::grpc" Disable TLS 1.3-specific unit tests in ssl_transport_security_test.cc. Fix number of TLS version tests to run. Merge pull request grpc#23609 from grpc/revert-23308-create-channel Revert "Move create_channel and credentials from ::grpc_impl to ::grpc" Merge pull request grpc#23567 from HannahShiSFB/23477 PHP: avoid destroy channel more than once Merge pull request grpc#23608 from matthewstevenson88/matt-disable-tls13 Disable TLS 1.3 in the C-core. Fix BUILD file for import. Member variable should not be a reference; causing crash when used in destructor. Merge pull request grpc#23575 from yashykt/chttp2transportcomment Comment formatting in chttp2_transport Merge pull request grpc#23610 from donnadionne/crash Member variable should not be a reference; causing crash when used in destructor. Merge pull request grpc#23612 from markdroth/xds_v3_build_fix Fix BUILD file for import. Passing repo manager to markdroth Merge pull request grpc#23624 from donnadionne/rm Passing repo manager to markdroth resolved merge conflicts with envoy v3 api

vjpai added area/core release notes: no Indicates if PR should not be in release notes labels Apr 23, 2020

vjpai force-pushed the core_server_cxx branch 8 times, most recently from be7d63d to 66016b6 Compare April 27, 2020 14:21

vjpai marked this pull request as ready for review April 27, 2020 15:08

vjpai requested a review from markdroth April 27, 2020 15:08

markdroth requested changes Apr 29, 2020

View reviewed changes

vjpai commented Apr 30, 2020

View reviewed changes

vjpai commented May 12, 2020

View reviewed changes

vjpai force-pushed the core_server_cxx branch from 8d3fe10 to 184d336 Compare June 25, 2020 17:18

vjpai commented Jun 25, 2020

View reviewed changes

vjpai force-pushed the core_server_cxx branch from 0a991e2 to 6af0e11 Compare June 26, 2020 09:53

vjpai commented Jun 26, 2020

View reviewed changes

markdroth requested changes Jun 26, 2020

View reviewed changes

vjpai commented Jun 29, 2020

View reviewed changes

markdroth requested changes Jul 14, 2020

View reviewed changes

vjpai commented Jul 16, 2020

View reviewed changes

vjpai force-pushed the core_server_cxx branch 2 times, most recently from ac95264 to d72f465 Compare July 17, 2020 08:31

markdroth requested changes Jul 17, 2020

View reviewed changes

vjpai commented Jul 17, 2020

View reviewed changes

markdroth approved these changes Jul 17, 2020

View reviewed changes

vjpai commented Jul 19, 2020

View reviewed changes

vjpai commented Jul 20, 2020

View reviewed changes

C++ify core server

9d79ca6

vjpai force-pushed the core_server_cxx branch from e9ce188 to 9d79ca6 Compare July 20, 2020 10:15

Make ABSL header block consistent with other code.

a6e541b

vjpai mentioned this pull request Jul 21, 2020

Typo fix #23571

Merged

vjpai merged commit 172f61a into grpc:master Jul 21, 2020

vjpai deleted the core_server_cxx branch July 21, 2020 23:50

Conversation

vjpai commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjpai commented Apr 27, 2020

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jun 2, 2020

Uh oh!

vjpai commented Jun 24, 2020

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai left a comment

Choose a reason for hiding this comment

Uh oh!

vjpai commented Jul 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vjpai commented Apr 23, 2020 •

edited

Loading