[EventEngine] WindowsEndpoint #31735

drfloob · 2022-11-22T00:38:39Z

Initial implementation, tested basic communication.

This also modifies the EventEngine TCP socket utils which were forked from iomgr for the PosixEventEngine, and makes them once again usable on other platforms.

Future work:

I expect that once the client is roughed out, the needs for two-stage shutdown and ref counting will be more obvious (and testable).

Initial sketch, all tests passing

Automated fix for refs/heads/win-ee-endpoint

ctiller · 2022-11-22T23:27:20Z

src/core/lib/event_engine/windows/resolved_address.cc

+}
+
+// TODO(hork): implement normalization
+absl::StatusOr<std::string> ResolvedAddressToString(


I count two implementations of this in the code base now, making this a third.

Given this is relatively complex and bug-prone code to write & maintain, I'd like us to have a plan to get down to one. I'd strongly prefer that we never exceed two -- can this implementation be merged with another to achieve that?

I will reunify the PosixEventEngine code.

This is done, but because of it, I think we'll need a cherrypick.

Correction: this does not require a cherrypick, I did a dry run and found no uses (which makes sense)

ctiller · 2022-11-22T23:28:36Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  char* wsa_message = gpr_format_message(wsa_error);
+  std::string message;
+  if (!custom_message.empty()) {
+    std::string message = absl::StrCat(wsa_message, ": ", custom_message);


remove the std::string here - we don't want to declare a new variable and then immediately throw it away

oops, thanks.

ctiller · 2022-11-22T23:29:37Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    case WSAECONNRESET:
+    case WSAECONNABORTED:
+    case WSA_OPERATION_ABORTED:
+      return StatusCreate(absl::StatusCode::kAborted, message, location, {});


agree we should be careful with status codes here - this could constitute an api change

Chatting with @veblush, we could try to make this change in iomgr and see what the end2end tests say about it (hoping they catch what we care about). It's unclear if a low-level windows or posix socket error ever bubbles up as the final grpc_status_code. The careful thing to do is keep status quo, so I'll do that for now, and experiment separately. Until the status migration is complete (e.g., int payloads are gone) and the EventEngine migration is done, I'm not sure how we could be truly confident that this change is safe.

ctiller · 2022-11-22T23:30:52Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  sockaddr addr;
+  int addr_len = sizeof(addr);
+  if (getsockname(socket_->socket(), &addr, &addr_len) < 0) {
+    GPR_ASSERT(false &&


I'd prefer gpr_log, abort() -- this isn't an assert

Done. There are 84 other uses of GPR_ASSERT(false && "Reason") in the codebase at the moment, it's a pattern/smell that can be cleaned up if you feel so inclined.

#31757 starts this work

ctiller · 2022-11-22T23:33:30Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  GRPC_EVENT_ENGINE_TRACE("WindowsEndpoint::%p reading", this);
+  // Prepare the WSABUF struct
+  WSABUF wsa_buffers[kMaxWSABUFCount];
+  // TODO(hork): use read hint instead of the default?


ideally we should not return from a read until the read hint is achieved (it's labelled min_progress_size in iomgr and that seems a better name)

if we do so, the calling code will assuredly call read again and not process the incoming memory until that limit is achieved anyway

whether that's important now or not I have no opinion, but ultimately we'll want to post at least min_progress_size bytes for an async read to avoid spurious wakeups from the kernel (we could post more, that would be fine)

Agreed. min_progress_size is currently unused in the Windows iomgr implementation, but we can improve upon that here. I've added the override.

ctiller · 2022-11-22T23:36:54Z

src/core/lib/event_engine/windows/windows_endpoint.h

+   public:
+    explicit BaseEventClosure(WindowsEndpoint* endpoint);
+    virtual void Run() = 0;
+    void SetCallback(absl::AnyInvocable<void(absl::Status)> cb) {


We always call SetCallback then SetSliceBuffer - suggest combining these into a single Init/Prime/Prepare call

ctiller · 2022-11-22T23:38:27Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    }
+  }
+  GPR_ASSERT(data->Count() <= UINT_MAX);
+  WSABUF local_buffers[kMaxWSABUFCount];


suggest absl::InlinedVector<WSABUF, kMaxWSABUFCount> here and eschewing the complex hand allocation/deallocation

will evaluate, thanks.

ctiller · 2022-11-22T23:40:47Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    buffers = allocated;
+  }
+  for (int i = 0; i < data->Count(); i++) {
+    auto slice = data->RefSlice(i);


meta: we need a way of getting this data out without referencing the contained slice - doing so is incredibly inefficient

I deleted a comment here along the lines of // DO NOT SUBMIT: implement PeekSlice, but did not want to complicate this PR with a public API change. I can do the public SliceBuffer change now in a separate PR.

There are also ways we could add the functionality internally as a non-member helper function around the c slice buffer, but I think it's probably best in the public API.

ctiller · 2022-11-22T23:44:06Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  if (status == 0) {
+    if (bytes_sent == data->Length()) {
+      // Write completed, exiting early
+      executor_->Run(


Interesting question I don't know the answer to: we do a synchronous write for efficiency here, but then need an extra allocation to call the callback.... does this render the synchronous path moot?

You're talking about allocations due to the size of the lambda capture? Even if so, this still saves a few hops through IOCP callbacks, so not quite moot, but it's not as good as the ExecCtx version we're replacing. We could instead capture this and add ref counting to the endpoint itself to minimize the size of the lambda.

ctiller · 2022-11-22T23:45:52Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    GPR_ASSERT(write_info->bytes_transferred() == buffer_->Length());
+  }
+  endpoint_->executor_->Run(
+      [cb = std::move(cb), status = std::move(status)]() mutable {


The read callback is made inline without going through the executor again... is there a reason this one also cannot?

Fixed. That was an oversight, they should both use the executor. But since IOCP is running its callbacks in an executor anyhow, they can both be called inline here.

ctiller · 2022-11-22T23:50:46Z

test/core/event_engine/windows/create_sockpair.h

@@ -22,6 +22,11 @@
 namespace grpc_event_engine {
 namespace experimental {

+sockaddr_in GetLoopbackAddress();


Pedant: prefer GetSomeIpv4LoopbackAddress -- rfc5735 has loopback as being all of 127.0.0.0/8 for ipv4

That's a better name. Done.

drfloob

Thanks for your review. I responded with a couple of questions, fixed most suggestions, and have one or two left to do.

drfloob · 2022-11-23T00:05:45Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    buffers = allocated;
+  }
+  for (int i = 0; i < data->Count(); i++) {
+    auto slice = data->RefSlice(i);


I deleted a comment here along the lines of // DO NOT SUBMIT: implement PeekSlice, but did not want to complicate this PR with a public API change. I can do the public SliceBuffer change now in a separate PR.

There are also ways we could add the functionality internally as a non-member helper function around the c slice buffer, but I think it's probably best in the public API.

drfloob · 2022-11-23T00:05:50Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  sockaddr addr;
+  int addr_len = sizeof(addr);
+  if (getsockname(socket_->socket(), &addr, &addr_len) < 0) {
+    GPR_ASSERT(false &&


Done. There are 84 other uses of GPR_ASSERT(false && "Reason") in the codebase at the moment, it's a pattern/smell that can be cleaned up if you feel so inclined.

drfloob · 2022-11-23T00:05:53Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  char* wsa_message = gpr_format_message(wsa_error);
+  std::string message;
+  if (!custom_message.empty()) {
+    std::string message = absl::StrCat(wsa_message, ": ", custom_message);


oops, thanks.

drfloob · 2022-11-23T00:19:34Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  if (status == 0) {
+    if (bytes_sent == data->Length()) {
+      // Write completed, exiting early
+      executor_->Run(


You're talking about allocations due to the size of the lambda capture? Even if so, this still saves a few hops through IOCP callbacks, so not quite moot, but it's not as good as the ExecCtx version we're replacing. We could instead capture this and add ref counting to the endpoint itself to minimize the size of the lambda.

drfloob · 2022-11-23T00:24:49Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    GPR_ASSERT(write_info->bytes_transferred() == buffer_->Length());
+  }
+  endpoint_->executor_->Run(
+      [cb = std::move(cb), status = std::move(status)]() mutable {


Fixed. That was an oversight, they should both use the executor. But since IOCP is running its callbacks in an executor anyhow, they can both be called inline here.

drfloob · 2022-11-23T00:33:09Z

test/core/event_engine/windows/create_sockpair.h

@@ -22,6 +22,11 @@
 namespace grpc_event_engine {
 namespace experimental {

+sockaddr_in GetLoopbackAddress();


That's a better name. Done.

drfloob · 2022-11-23T00:39:24Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    case WSAECONNRESET:
+    case WSAECONNABORTED:
+    case WSA_OPERATION_ABORTED:
+      return StatusCreate(absl::StatusCode::kAborted, message, location, {});


Chatting with @veblush, we could try to make this change in iomgr and see what the end2end tests say about it (hoping they catch what we care about). It's unclear if a low-level windows or posix socket error ever bubbles up as the final grpc_status_code. The careful thing to do is keep status quo, so I'll do that for now, and experiment separately. Until the status migration is complete (e.g., int payloads are gone) and the EventEngine migration is done, I'm not sure how we could be truly confident that this change is safe.

drfloob · 2022-11-23T00:48:04Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    case WSAECONNRESET:
+    case WSAECONNABORTED:
+    case WSA_OPERATION_ABORTED:
+      return StatusCreate(absl::StatusCode::kAborted, message, location, {});


drfloob · 2022-11-23T00:57:57Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    }
+  }
+  GPR_ASSERT(data->Count() <= UINT_MAX);
+  WSABUF local_buffers[kMaxWSABUFCount];


will evaluate, thanks.

drfloob · 2022-11-23T01:02:36Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+  GRPC_EVENT_ENGINE_TRACE("WindowsEndpoint::%p reading", this);
+  // Prepare the WSABUF struct
+  WSABUF wsa_buffers[kMaxWSABUFCount];
+  // TODO(hork): use read hint instead of the default?


Agreed. min_progress_size is currently unused in the Windows iomgr implementation, but we can improve upon that here. I've added the override.

Automated fix for refs/heads/win-ee-endpoint

ctiller · 2022-11-23T20:55:03Z

src/core/lib/event_engine/windows/windows_endpoint.cc

+    auto slice = data->RefSlice(i);
+    GPR_ASSERT(slice.size() <= ULONG_MAX);
+    buffers[i].len = slice.size();
+    buffers[i].buf = (char*)slice.begin();


nit: reinterpret_cast -- avoid c-style casts

Automated fix for refs/heads/win-ee-endpoint

* [EventEngine] WindowsEndpoint Initial sketch, all tests passing * Port fix from #28432 * GPR_WINDOWS guard * use MemoryAllocator::MakeReservation for allocated buffers * better logging (respect slice length) * Automated change: Fix sanity tests * improvements * Automated change: Fix sanity tests * InlinedVector<WSABUF, kMaxWSABUFCount> * initial attempt at socket util reunification * posix fixes + local run of sanitize.sh * posix socket includes * fix * Automated change: Fix sanity tests * remove unused include (breaks windows) * remove stale comment Co-authored-by: drfloob <drfloob@users.noreply.github.com>

drfloob and others added 7 commits November 21, 2022 23:47

[EventEngine] WindowsEndpoint

5ed4ffd

Initial sketch, all tests passing

Merge remote-tracking branch 'upstream/master' into win-ee-endpoint

98af863

Port fix from grpc#28432

c8da1ce

GPR_WINDOWS guard

5881a5f

use MemoryAllocator::MakeReservation for allocated buffers

e7ded3c

better logging (respect slice length)

823af36

Automated change: Fix sanity tests

016ac0a

drfloob added the release notes: no Indicates if PR should not be in release notes label Nov 22, 2022

drfloob requested a review from ctiller November 22, 2022 00:38

github-actions bot added the lang/core label Nov 22, 2022

drfloob requested a review from veblush November 22, 2022 00:44

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Nov 22, 2022

Merge pull request #352 from drfloob/create-pull-request/patch-823af36

815529a

Automated fix for refs/heads/win-ee-endpoint

ctiller reviewed Nov 22, 2022

View reviewed changes

ctiller requested changes Nov 22, 2022

View reviewed changes

improvements

23fe8fd

drfloob commented Nov 23, 2022

View reviewed changes

drfloob and others added 5 commits November 23, 2022 01:18

Automated change: Fix sanity tests

40fcd31

Merge pull request #353 from drfloob/create-pull-request/patch-23fe8fd

7fb38f8

Automated fix for refs/heads/win-ee-endpoint

InlinedVector<WSABUF, kMaxWSABUFCount>

2961515

initial attempt at socket util reunification

d11d5e0

posix fixes + local run of sanitize.sh

8641563

drfloob requested a review from markdroth as a code owner November 23, 2022 19:57

github-actions bot added lang/c++ lang/Python labels Nov 23, 2022

posix socket includes

9bc8b1e

grpc-checks bot added bloat/low and removed bloat/none labels Nov 23, 2022

grpc-checks bot added bloat/none and removed bloat/low labels Nov 23, 2022

drfloob and others added 2 commits November 23, 2022 12:43

fix

7accedf

Automated change: Fix sanity tests

18ff822

ctiller reviewed Nov 23, 2022

View reviewed changes

ctiller approved these changes Nov 23, 2022

View reviewed changes

grpc-checks bot added bloat/low and removed bloat/none labels Nov 23, 2022

drfloob added 2 commits November 23, 2022 14:04

Merge pull request #358 from drfloob/create-pull-request/patch-7accedf

f91af1d

Automated fix for refs/heads/win-ee-endpoint

remove unused include (breaks windows)

1e91f53

grpc-checks bot added bloat/none and removed bloat/low labels Nov 23, 2022

drfloob added 2 commits November 28, 2022 13:00

Merge branch 'master' into win-ee-endpoint

2769562

remove stale comment

debcbf4

drfloob merged commit 557e558 into grpc:master Nov 28, 2022

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Nov 29, 2022

[EventEngine] WindowsEndpoint #31735

[EventEngine] WindowsEndpoint #31735

Conversation

drfloob commented Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob left a comment

Choose a reason for hiding this comment

drfloob Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drfloob commented Nov 22, 2022 •

edited

Loading

drfloob Nov 23, 2022 •

edited

Loading

drfloob Nov 23, 2022 •

edited

Loading

drfloob Nov 23, 2022 •

edited

Loading

drfloob Nov 23, 2022 •

edited

Loading