Thanks Nico! I'm excited to see this come to fruition. I've done a partial first pass, and still need to review the meat of the uv implementation, slice, and slice_buffer.

Reviewed 36 of 39 files at r1, all commit messages.
Reviewable status: 36 of 39 files reviewed, 8 unresolved discussions (waiting on @ctiller, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

include/grpc/event_engine/endpoint_config.h, line 42 at r1 (raw file):

  /// \a key in the EndpointConfig, an \a absl::monostate type will be
  /// returned. Caller does not take ownership of resulting value.
  virtual Setting Get(absl::string_view key) const { abort(); }

Why did the interface (now base class) need an implementation?

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

#ifdef GRPC_USE_EVENT_ENGINE
#undef GPR_SUPPORT_CHANNELS_FROM_FD
#define GRPC_ARES 1

I think this can be ignored for now, but note that it will cause some tests to fail. Ares is not support on iOS or custom iomgrs yet (gevent, specifically, had issues in the past). I'm refactoring the ares code now, separately.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 546 at r1 (raw file):

    return err;
  }
  driver->work_serializer = std::move(work_serializer);

FYI, @markdroth had rejected this cleanup when I introduced it months ago. Probably best to revert.

src/core/ext/transport/chttp2/transport/chttp2_slice_allocator.cc, line 27 at r1 (raw file):

#include "src/core/lib/transport/error_utils.h"

extern grpc_core::TraceFlag grpc_tcp_trace;

This appears to be unused.

src/core/lib/iomgr/event_engine/promise.h, line 29 at r1 (raw file):

/// .wait() and .notify().
template <typename T>
class Promise {

@ctiller are the other gRPC promises ready to be used yet?

src/core/lib/iomgr/event_engine/promise.h, line 35 at r1 (raw file):

  T& Get() {
    absl::MutexLock lock(&mu_);
    if (!set_) {

If we're going to rely on this, it might be worth adding a smoke test for the race condition of Get before Set (a bug caught recently).

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1 at r1 (raw file):

#include <uv.h>

Run the sanitizer/auto-fix on this file. Needs a copyright header.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 5 at r1 (raw file):

#include <atomic>
#include <functional>
#include <future>

This is a banned construct, see tools/run_tests/sanity/cpp_banned_constructs.sh.

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

#include <memory.h>

#include <future>

This is unused, and regardless, <future> is banned in the codebase at the moment.

This looks like a fantastic start, Nico!

Please don't let the number of comments here disturb you -- most of them are cosmetic issues. There are just a few comments that are more substantive, such as the ones about using a more nested class structure to make the overal map of the code clearer to the reader.

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

Reviewed 39 of 39 files at r1, all commit messages.
Reviewable status: all files reviewed, 61 unresolved discussions (waiting on @ctiller, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

BUILD, line 1257 at r1 (raw file):

        "src/core/lib/iomgr/event_engine/tcp.cc",
        "src/core/lib/iomgr/event_engine/timer.cc",
        "src/core/lib/iomgr/event_engine/uv/impl.cc",

Would it make sense to put this libuv-base EE impl in its own build target, so that it's not required for applications that want to provide their own EE impl?

include/grpc/event_engine/slice.h, line 38 at r1 (raw file):

/// slice pointing to a subset of another slice.
class Slice final {
  friend class SliceBuffer;

Could we avoid needing this somehow? It looks like it's mainly used for move semantics even without using an rvalue reference. Could we restructure things to avoid that somehow?

If this is needed, please move it down into the private: section.

include/grpc/event_engine/slice.h, line 78 at r1 (raw file):

  /// Assignment, reference count is unchanged.
  Slice& operator=(Slice other) {

It seems a little counter-intuitive to use move semantics for a normal assignment operator. Maybe we want two versions of this, one for copy-assignment and one for move-assignment?

include/grpc/event_engine/slice.h, line 109 at r1 (raw file):

  /// Raw C slice. Caller needs to call grpc_slice_unref when done.
  grpc_slice c_slice() const { return grpc_slice_ref(slice_); }

Please explicitly document the fact that this isn't a stable API, just to make sure that people don't get confused about it.

include/grpc/event_engine/slice.h, line 109 at r1 (raw file):

  /// Raw C slice. Caller needs to call grpc_slice_unref when done.
  grpc_slice c_slice() const { return grpc_slice_ref(slice_); }

Would it make sense for this to return const grpc_slice& and make it the caller's responsibility to take a new ref if they need to?

include/grpc/event_engine/slice.h, line 112 at r1 (raw file):

 private:
  friend class ByteBuffer;

We don't have any such class in the EE API. I assume this was copied from the C++ Slice API accidentally. :)

include/grpc/event_engine/slice_allocator.h, line 25 at r1 (raw file):

#include <grpc/event_engine/slice_buffer.h>

// forward-declaring an internal struct, not used publicly.

Doesn't look like any of these are actually needed.

include/grpc/event_engine/slice_buffer.h, line 45 at r1 (raw file):

class SliceBuffer final {
 public:
  SliceBuffer(grpc_slice_buffer* sb) : sb_(sb) {}

Please add a TODO that we need to change this API to not depend on the C-core type before de-experimentalizing the EE API. I think we'll want this to happen as part of eliminating the iomgr API.

include/grpc/event_engine/slice_buffer.h, line 49 at r1 (raw file):

  SliceBuffer(SliceBuffer&& other) : sb_(other.sb_) { other.sb_ = nullptr; }

  // This easier to write than an iterator, and essentially does the same, in a

Couldn't the iterator return references to the slices embedded in the SliceBuffer, so that there's no copy involved? I think most STL iterators return references, don't they?

include/grpc/event_engine/slice_buffer.h, line 68 at r1 (raw file):

  }

  void Add(Slice slice) {

It seems odd to me that this API takes ownership of the slice without it being passed in as an rvalue reference.

include/grpc/event_engine/slice_buffer.h, line 73 at r1 (raw file):

  }

  size_t AddIndexed(Slice slice) {

Same here -- and this is identical to the next method, which does take an rvalue reference. Why do we need both?

include/grpc/event_engine/slice_buffer.h, line 119 at r1 (raw file):

  size_t Length() { return sb_->length; }

  grpc_slice_buffer* RawSliceBuffer() { return sb_; }

Suggest calling this c_slice_buffer().

include/grpc/event_engine/slice_buffer.h, line 119 at r1 (raw file):

  size_t Length() { return sb_->length; }

  grpc_slice_buffer* RawSliceBuffer() { return sb_; }

Please document that this is not a stable API.

src/core/ext/transport/chttp2/transport/chttp2_slice_allocator.cc, line 25 at r1 (raw file):

#include "src/core/ext/transport/chttp2/transport/chttp2_slice_allocator.h"
#include "src/core/lib/iomgr/resource_quota.h"
#include "src/core/lib/transport/error_utils.h"

Doesn't lool like this is needed.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 10 at r1 (raw file):

#include "absl/strings/str_format.h"

#include "grpc/event_engine/event_engine.h"

This include should use angle brackets.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 33 at r1 (raw file):

// Probably belongs to a different utility file elsewhere, but wasn't sure if
// we even want to keep it eventually.
static void hexdump(const std::string& prefix, const void* data_, size_t size) {

static not needed, since we're already in an anonymous namespace.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 33 at r1 (raw file):

// Probably belongs to a different utility file elsewhere, but wasn't sure if
// we even want to keep it eventually.
static void hexdump(const std::string& prefix, const void* data_, size_t size) {

Can you use absl::BytesToHexString() for this?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 68 at r1 (raw file):

}

constexpr size_t READ_BUFFER_SIZE = 4096;

This should be named kReadBufferSize, as per:

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

src/core/lib/iomgr/event_engine/uv/impl.cc, line 84 at r1 (raw file):

/// functors, buffers, etc.
////////////////////////////////////////////////////////////////////////////////
class LibuvWrapperBase {

It looks like the purpose of this class is to provide ref-counting for libuv objects. If so, why not use our existing ref-counting classes for this? That way, engineers on grpc-team who are already familiar with those classes don't need to figure out a new API.

Internally, see go/grpc-c-core-ref-counted-types-update for an overview.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 97 at r1 (raw file):

  // Registers the libuv handle into the libuv loop. This needs to be called
  // from the corresponding libuv Thread.
  void RegisterUnsafe(LibuvEventEngine* engine);

It looks like you're using Unsafe as a suffix for a number of methods to mean "must be run in libuv thread". How about using "InLibuvThread" instead, since that makes the intent a bit clearer?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 133 at r1 (raw file):

  }

  uv_tcp_t tcp_;

Data members must be private, as per:

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

src/core/lib/iomgr/event_engine/uv/impl.cc, line 141 at r1 (raw file):

////////////////////////////////////////////////////////////////////////////////
/// The derived class to wrap a LibUV TCP listener handle. Its API will
/// closely match that of the Event Engine listener. Its only consumer should

If the only user of this class is the LibuvListener class, how about nesting this in the private section of LibuvListener, so that its scope is clear? That may also eliminate the need for the "friend" statement, because then you can just make all of the methods public.

Same thing for all of these wrapper types.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 145 at r1 (raw file):

////////////////////////////////////////////////////////////////////////////////
class LibuvListenerWrapper final : public LibuvWrapperBase {
  friend class LibuvListener;

Please add private: here (or make the methods public: and the data members private:).

Same thing for all of these classes.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 148 at r1 (raw file):

  LibuvListenerWrapper(
      grpc_event_engine::experimental::EventEngine::Listener::AcceptCallback

Suggest adding using grpc_event_engine::experimental::EventEngine; at the top of the file, so that we don't have to be so verbose when referring to EE types.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 171 at r1 (raw file):

  // called from the base class, which is when we can properly invoke the
  // shutdown callback.
  virtual ~LibuvListenerWrapper() { on_shutdown_(absl::OkStatus()); };

Dtor should come before other methods, as per:

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

Same thing throughout.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 286 at r1 (raw file):

  virtual absl::Status Start() override;

  LibuvListenerWrapper* uvTCP_ = nullptr;

Please call this uv_tcp_, as per naming rules:

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

In this case, it might be even clearer to call this something like listener_wrapper_, or maybe even just impl_.

Same for all of these classes.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 331 at r1 (raw file):

  //
  // Returns true if the accept was successful.
  bool AcceptUnsafe(LibuvEventEngine* engine, uv_stream_t* server) {

As per the style guide, large method definitions should not be inlined in the class declaration:

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

In this case, I think it would really help to move the definitions of most methods out of the class declarations, so that the reader can first get an idea of what the class structure is and then go read the implementation.

Same thing throughout.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 331 at r1 (raw file):

  //
  // Returns true if the accept was successful.
  bool AcceptUnsafe(LibuvEventEngine* engine, uv_stream_t* server) {

Why is this a method on LibuvEndpoint()? Wouldn't it make more sense as a method on LibuvListener?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 334 at r1 (raw file):

    RegisterUnsafe(engine);
    int r;
    r = uv_accept(server, reinterpret_cast<uv_stream_t*>(&uvTCP_->tcp_));

Can combine with previous line.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 351 at r1 (raw file):

 private:
  // The LibuvEventEngine pointer is tucked away into the uv_loop_t user data.

Shouldn't each of these nested objects (endpoint, listener, DNS resolver, etc) actually hold a ref to the main EE object? Otherwise, if they accidentally outlive the main object, it might get destroyed too early.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 409 at r1 (raw file):

/// The LibUV Event Engine itself. It implements an EventEngine class.
////////////////////////////////////////////////////////////////////////////////
class LibuvEventEngine final

A couple of general comment about the class organization here:

Instead of having a bunch of independent classes all defined at the same level, I think it would be really helpful if this code used nesting to make the scope of any given class more clear. For example, I think that basically all of the classes in this file should be nested inside of this LibuvEventEngine class, in the private section. I think that will avoid the need for a lot of the friend declarations in these classes. The nested classes will be able to directly access the parent's data members, and the nested classes can have any member that needs to be called by the parent be public.

I think it would also be useful to move the class declarations to a .h file, at least those that correspond to classes in the EE API. For purely internal classes (like the uv wrapper classes), it's fine to just forward-declare them in the .h file and have their declarations be in the .cc file.

So, for example, we could have a header file for this that has something like this:

class LibuvEventEngine final : public EventEngine {
 public:
  // ...ctor, dtor, public method declarations...

 private:
  class LibuvTask;  // Forward declaration, defined in .cc file.

  class LibuvEndpoint final : public EventEngine::Endpoint {
   public:
    // ...public method declarations...
    // ...other methods needed by LibuvEventEngine class...
   private:
    class LibuvEndpointWrapper;  // Forward declaration, defined in .cc file.
    // ...data members...
  };

  class LibuvListener final : public EventEngine::Listener {
   public:
    // ...public method declarations...
    // ...other methods needed by LibuvEventEngine class...
   private:
    class LibuvListenerWrapper;  // Forward declaration, defined in .cc file.
    // ...data members...
  };

  class LibuvDNSResolver final : public EventEngine::DNSResolver {
   public:
    // ...public method declarations...
    // ...other methods needed by LibuvEventEngine class...
   private:
    class LibuvLookupTask;  // Forward declaration, defined in .cc file.
    // ...data members...
  };

  // ...data members...
};

src/core/lib/iomgr/event_engine/uv/impl.cc, line 411 at r1 (raw file):

class LibuvEventEngine final
    : public grpc_event_engine::experimental::EventEngine {
  // Since libuv is single-threaded and not thread-safe, we will be running

Private declarations should go after public ones, as per:

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

src/core/lib/iomgr/event_engine/uv/impl.cc, line 421 at r1 (raw file):

    typedef std::function<void(LibuvEventEngine*)> functor;
    SchedulingRequest(functor&& f) : f_(std::move(f)) {}
    functor f_;

Struct data member names, unlike class data member names, should not have a trailing underscore:

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

src/core/lib/iomgr/event_engine/uv/impl.cc, line 454 at r1 (raw file):

  // will have a special async event which is the only piece of API that's
  // marked as thread-safe.
  void Schedule(SchedulingRequest::functor&& f) {

I think this method should be private.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 454 at r1 (raw file):

  // will have a special async event which is the only piece of API that's
  // marked as thread-safe.
  void Schedule(SchedulingRequest::functor&& f) {

Maybe call this RunInLibuvThread()?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 464 at r1 (raw file):

  }

  uv_loop_t* GetLoop() { return &loop_; }

If you take my suggestion above about nesting the other classes inside of this one, then I think this method won't be needed, because the other classes will have direct access to loop_.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 476 at r1 (raw file):

      SchedulingRequest* node =
          reinterpret_cast<SchedulingRequest*>(queue_.PopAndCheckEnd(&empty));
      if (!node) continue;

Prefer node != nullptr, to make it clear that this is a pointer and not a bool.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 502 at r1 (raw file):

    // Setting up the loop.
    int r = 0;

Can declare this two lines down, where it's actually used.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 543 at r1 (raw file):

                this);
      }
      ctx.Flush();

Don't we need to also flush callback_exec_ctx here?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 558 at r1 (raw file):

          slice_allocator_factory) override {
    std::unique_ptr<LibuvListener> ret = absl::make_unique<LibuvListener>(
        on_accept, on_shutdown, args, std::move(slice_allocator_factory));

I think this should use std::move() for on_accept and on_shutdown.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 566 at r1 (raw file):

    // until this is finished to return. Any subsequent call on the returned
    // listener will schedule more work after this one.
    Schedule([&ret](LibuvEventEngine* engine) { ret->RegisterUnsafe(engine); });

Doesn't this require that the newly created LibuvListener object continue to exist until after this callback runs in the libuv loop thread? What prevents the caller from immediately freeing the returned object before this callback runs in the other thread?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 596 at r1 (raw file):

  virtual void TryCancel(TaskHandle handle) override;

  virtual void Shutdown(Callback on_shutdown_complete) override {

I think this method has been removed from the EE API. We need to trigger shutdown from the dtor instead.

To make this work, we probably need to actually move most of the functionality into a child object that is ref-counted, and have the parent class just be a proxy object that calls the child object's Shutdown() method when the parent object gets destroyed.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 653 at r1 (raw file):

  // atomics as tags in the second key slot, but we can get rid of the maps.
  //
  // TODO(nnoble): remove the maps, and fold the pointers into the keys,

Any reason not to do this now?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 655 at r1 (raw file):

  // TODO(nnoble): remove the maps, and fold the pointers into the keys,
  // alongside the ABA tag.
  std::atomic<intptr_t> taskKey_;

These should be called task_key_, lookup_task_key_, task_map_, and lookup_task_map_, respectively.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 829 at r1 (raw file):

  // Unlike other callbacks in this code, this one is guaranteed to only be
  // called once by libuv.
  void LibuvResolverCallback(int status, struct addrinfo* res) {

Please move this to the top of the private: section, ahead of the data members.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 877 at r1 (raw file):

        &connect_, &uvTCP_->tcp_, addr.address(),
        [](uv_connect_t* req, int status) {
          LibuvEndpoint* epRaw = reinterpret_cast<LibuvEndpoint*>(req->data);

This should be named ep_raw.

Or better yet, remove this variable entirely and just wrap it directly in the smart pointer:

std::unique_ptr<LibuvEndpoint> ep(reinterpret_cast<LibuvEndpoint*>(req->data));

src/core/lib/iomgr/event_engine/uv/impl.cc, line 967 at r1 (raw file):

    }

    sockaddr_storage boundAddr;

Please call these bound_addr and addr_len, as per style guide naming rules.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1019 at r1 (raw file):

  grpc_event_engine::experimental::Promise<absl::Status> ret;
  GetEventEngine()->Schedule([this, &ret](LibuvEventEngine* engine) {
    int r;

This can be combined with the next (non-comment) line.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1031 at r1 (raw file):

          std::unique_ptr<LibuvEndpoint> e = absl::make_unique<LibuvEndpoint>(
              l->args_, l->slice_allocator_factory_->CreateSliceAllocator(
                            "TODO(nnoble): get peer name"));

Anything blocking us from doing this?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1054 at r1 (raw file):

  });
  auto status = ret.Get();
  // until we can handle "SO_REUSEPORT", always return OkStatus.

Seems like this is something we need to resolve before we can merge this, right?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1170 at r1 (raw file):

  // set them all up with all the pointers and sizes from the slices themselves.
  size_t count = data->Count();
  tcp->write_bufs_ = new uv_buf_t[count];

Could we use absl::InlinedVector<> here to avoid some allocation? Or maybe even just std::vector<> with some pre-reserved capacity, which we reuse between writes?

test/core/event_engine/slice_buffer_test.cc, line 62 at r1 (raw file):

}

TEST(SliceBufferTest, DeletingSliceBufferDoesNotAffectInternalBuffer) {}

Looks like we're missing some tests here.

test/core/event_engine/slice_test.cc, line 26 at r1 (raw file):

using ::grpc_event_engine::experimental::Slice;

TEST(SliceTest, AssignmentConstructorLeavesOriginalSliceUnchanged) {

I think we need more tests here as well.

Reviewable status: all files reviewed, 62 unresolved discussions (waiting on @ctiller, @drfloob, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

BUILD, line 1257 at r1 (raw file):

Sure, this was easier this way. Any name suggestion for the new target, @drfloob ?

include/grpc/event_engine/endpoint_config.h, line 42 at r1 (raw file):

Settings are used as a first class entity, both in parameters and in the classes. If we wanted to get rid of this, we need the API to change those to unique pointers.

include/grpc/event_engine/slice.h, line 112 at r1 (raw file):

Right. The whole file was copied over basically, just with a different namespace. The other comments apply for this too in the sense that they weren't brand new APIs.

include/grpc/event_engine/slice_allocator.h, line 25 at r1 (raw file):

Done.

include/grpc/event_engine/slice_buffer.h, line 45 at r1 (raw file):

Done.

include/grpc/event_engine/slice_buffer.h, line 49 at r1 (raw file):

Right, it needs to avoid the pitfall of thinking that just using a Slice itself is fine, because the comments around Slices keep saying that doing copies of them is cheap and should be what people use. I'll augment the comment with this.

include/grpc/event_engine/slice_buffer.h, line 119 at r1 (raw file):

Done.

include/grpc/event_engine/slice_buffer.h, line 119 at r1 (raw file):

Done.

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

Symbols wouldn't be found otherwise.

src/core/ext/filters/client_channel/resolver/dns/c_ares/grpc_ares_wrapper.cc, line 546 at r1 (raw file):

Ack.

src/core/ext/transport/chttp2/transport/chttp2_slice_allocator.cc, line 25 at r1 (raw file):

Done.

src/core/ext/transport/chttp2/transport/chttp2_slice_allocator.cc, line 27 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/promise.h, line 29 at r1 (raw file):

Let's use those for now.

src/core/lib/iomgr/event_engine/promise.h, line 35 at r1 (raw file):

Ack.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 5 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 10 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 33 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 68 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 97 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 148 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 421 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 454 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 476 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 502 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 543 at r1 (raw file):

Good question, I don't remember why I didn't have to, but I'll give it a go.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 558 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 655 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 967 at r1 (raw file):

Done.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1054 at r1 (raw file):

Not necessarily. This will make the code fail later in case we didn't actually bind the port if something was already there.

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

Done. Was a leftover :)

Reviewable status: 31 of 39 files reviewed, 62 unresolved discussions (waiting on @ctiller, @drfloob, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

BUILD, line 1257 at r1 (raw file):

"grpc_event_engine_libuv"? Feel free to choose one, Nico!

Reviewable status: 31 of 39 files reviewed, 59 unresolved discussions (waiting on @ctiller, @drfloob, @gnossen, @jtattermusch, @markdroth, @nicolasnoble, and @veblush)

include/grpc/event_engine/endpoint_config.h, line 42 at r1 (raw file):

I think you're right, they should be unique_ptr. Do you want to make that change in this PR? I believe you need to merge the latest master, but there are only 2 signatures that need to be changed, no logic changes required yet afaict.

src/core/lib/iomgr/event_engine/promise.h, line 29 at r1 (raw file):

You mean we should use this smaller Promise implementation for now, instead of the new stuff, right? I think that's what we last discussed.

Reviewed 2 of 7 files at r2, 4 of 5 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @ctiller, @drfloob, @gnossen, @jtattermusch, @nicolasnoble, and @veblush)

BUILD, line 1257 at r1 (raw file):

That name sounds good to me.

include/grpc/event_engine/slice.h, line 112 at r1 (raw file):

Understood. But let's clean this stuff up before this lands.

include/grpc/event_engine/slice_allocator.h, line 25 at r1 (raw file):

I don't see a change here.

include/grpc/event_engine/slice_buffer.h, line 45 at r1 (raw file):

Please make this a TODO comment. If it doesn't say TODO, we won't know to go back and do it later. :)

include/grpc/event_engine/slice_buffer.h, line 49 at r1 (raw file):

Right, so doesn't that mean that we can provide an iterator interface instead of Enumerate()?

src/core/lib/iomgr/event_engine/uv/impl.cc, line 171 at r1 (raw file):

Looks like you fixed this here, but not for other classes.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 543 at r1 (raw file):

It would matter only if you test it with the C++ callback API, since that's the only thing that uses the ApplicationCallbackExecCtx.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 1054 at r1 (raw file):

Okay. But we do need to resolve this before we can actually use this code on the server side. If we're going to handle that in a separate PR, please make this a TODO comment.

src/core/lib/iomgr/event_engine/uv/impl.cc, line 39 at r6 (raw file):

extern grpc_core::TraceFlag grpc_tcp_trace;

using namespace grpc_event_engine::experimental;

The style guide prohibits namespace-level using namespace statements:

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

Please use using grpc_event_engine::experimental::EventEngine; instead.

Some thoughts on slice apis, since I'm currently doing one for grpc_core - let's get alignment on what these look like.