EventEngine::Closure

[drfloob]

This introduces the new EventEngine::Closure type, and new cancellation semantics. I'll update the gRFC and ping the mailing list with a notification of it, since this is a significant change.

I've also fixed a few bugs in the EventEngine iomgr implementation that had gone unnoticed.

CC @Vignesh2208 @nicolasnoble

---

This change is 

[markdroth]

The API changes look good. I have a few comments about the implementation.

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ctiller and @drfloob)

---

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

    defined(GRPC_EVENT_ENGINE_REPLACE_EXEC_CTX)
  grpc_iomgr_event_engine()->Run(
      GrpcClosureToCallbackWithStatus(closure, error), {});

Does this actually work? EventEngine::Run() now takes std::function<void()>, not std::function<void(absl::Status)>, but GrpcClosureToCallbackWithStatus() returns the latter type.

---

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

/// Creates callbacks that take an error status argument.
std::function<void(absl::Status)> GrpcClosureToCallbackWithStatus(

This API doesn't look quite right to me.

It looks like we're using this in two separate places, each of which actually needs something slightly different:

1. The on_shutdown parameter to CreateListener(). This is a case where the EE wants a std::function<void(absl::Status)> and will generate the absl::Status parameter itself. In this case, there's no need to pass in the grpc_error_handle, because it won't be used; we just need the std::function<> to convert the absl::Status returned by the EE impl to a grpc_error_handle before invoking the grpc_closure callback.
2. In ExecCtx::Run(), where we delegate to EventEngine::Run(). This is a case where the EE wants a std::function<void()> (note: no absl::Status parameter), and we want to just transparently pass the grpc_error_handle that was passed to ExecCtx::Run() to the resulting callback. In this case, the EE impl does not know or care about the grpc_error_handle; it's essentially just a pre-bound parameter in the std::function<>.

I think these two cases should be two different functions.

---

src/core/lib/iomgr/event_engine/timer.cc, line 41 at r1 (raw file):

  auto handle = timer->ee_task_handle;
  if (!grpc_iomgr_event_engine()->Cancel(handle)) {
    // TODO(hork): This likely needs to be handled by the caller. An iomgr API

I think we can handle this here:

• In timer_init(), store closure in timer->closure.
• Here in timer_cancel(), if EventEngine::Cancel() returns true, then we can call ExecCtx::Run(timer->closure, GRPC_ERROR_CANCELLED).

[drfloob]

Thanks Mark, PTAL. All comments are addressed, and it now builds fine with bazel build --cxxopt='-DGRPC_USE_EVENT_ENGINE=1' --cxxopt="-DGRPC_EVENT_ENGINE_REPLACE_EXEC_CTX" ...

I wrote a simple Closure implementation, but stashed it for later when we have an EventEngine implementation to test against.

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

---

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

Previously, markdroth (Mark D. Roth) wrote…

Does this actually work? EventEngine::Run() now takes std::function<void()>, not std::function<void(absl::Status)>, but GrpcClosureToCallbackWithStatus() returns the latter type.

Right, that wouldn't have worked. Fixed.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This API doesn't look quite right to me.

It looks like we're using this in two separate places, each of which actually needs something slightly different:

1. The on_shutdown parameter to CreateListener(). This is a case where the EE wants a std::function<void(absl::Status)> and will generate the absl::Status parameter itself. In this case, there's no need to pass in the grpc_error_handle, because it won't be used; we just need the std::function<> to convert the absl::Status returned by the EE impl to a grpc_error_handle before invoking the grpc_closure callback.
2. In ExecCtx::Run(), where we delegate to EventEngine::Run(). This is a case where the EE wants a std::function<void()> (note: no absl::Status parameter), and we want to just transparently pass the grpc_error_handle that was passed to ExecCtx::Run() to the resulting callback. In this case, the EE impl does not know or care about the grpc_error_handle; it's essentially just a pre-bound parameter in the std::function<>.

I think these two cases should be two different functions.

Thanks. There are 3 places we need closure conversions, and all have different requirements. I've split out 3 conversion functions:

1. std::function<void(Status)> GrpcClosureToStatusCallback(closure) with no pre-bound error. Used in CreateListener call, as you pointed out.
2. std::function <void()> GrpcClosureToCallback(callback, error) with a pre-bound argument. Used in ExecCtx::Run, as you pointed out.
3. std::function <void()> GrpcClosureToCallback(callback) used in timer_init, without need for a bound error argument.

If we didn't mind binding an extra (possibly unnecessary) grpc_error_handle bound in the timer callback, we could reuse version 2 for timers.

---

src/core/lib/iomgr/event_engine/timer.cc, line 41 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think we can handle this here:

• In timer_init(), store closure in timer->closure.
• Here in timer_cancel(), if EventEngine::Cancel() returns true, then we can call ExecCtx::Run(timer->closure, GRPC_ERROR_CANCELLED).

Nice. That lets us defer migrating callers to the new cancelation flow, which is helpful to say the least.

[markdroth]

Looks good. Just one remaining comment.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ctiller and @drfloob)

---

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

  class Closure {
   public:
    // Closure's are an interface, and thus non-copyable.

Why are we making this copyable now?

[drfloob]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ctiller and @markdroth)

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why are we making this copyable now?

The best alternative is to explicitly define a default constructor for the interface, which is not constructible (the default is implicitly deleted). I'll add that in, and make it non-copyable again. That's maybe preferable to allowing accidental copies of the Closure ... but I'm not certain the type should be non-copyable. gRPC doesn't have a use for copied Closures at the moment, but we're forcing that restriction on all public uses as well by putting it in the interface.

[markdroth]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ctiller)

---

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

Previously, drfloob (AJ Heller) wrote…

The best alternative is to explicitly define a default constructor for the interface, which is not constructible (the default is implicitly deleted). I'll add that in, and make it non-copyable again. That's maybe preferable to allowing accidental copies of the Closure ... but I'm not certain the type should be non-copyable. gRPC doesn't have a use for copied Closures at the moment, but we're forcing that restriction on all public uses as well by putting it in the interface.

Given that the whole point of introducing the Closure type is to be able to use a pointer to the object as an identity, allowing the object to be copied would be counter to the intended use.

Anyway, this looks good.

[drfloob]

Errors:

• Artifact build is a timeout
• Basic tests python windows is a timeout
• Bazel C/C++ Dbg MacOS actually succeeded, but is reporting failure here. See the Details link.