Introduce CFRunLoop based iomgr by muxi · Pull Request #22423 · grpc/grpc

muxi · 2020-03-19T20:37:22Z

Use a CFRunLoop based iomgr for CFStream on apple platforms. Workaround for the Apple bug where CFStream gets stuck on read.

The new iomgr spawns a global thread to handle all the CFStream callbacks. Network events are driven by the global thread. The pollset is a dummy conditional variable blocked on pollset_work and can be awaken by pollset_kick.

The CFStream networking layer is supposed to be included on iOS-targeted builds but not MacOS-targeted builds. It is supposed to be specified in the header port_platform.h. Specifying it for Mac in .bzl file is not the right thing to do. Fixing this issue in the PR as well.

muxi · 2020-03-20T21:19:38Z

Not ready for review yet...

muxi · 2020-04-02T22:08:53Z

@guantaol ping

guantaol

General comment: please add some comments for the changes in those .cc files and .h files. I do not think I can understand what you want to achieve without comments.

guantaol · 2020-04-03T22:46:21Z

src/core/lib/iomgr/ev_apple.cc

+  grpc_core::CondVar cv;
+};
+
+static GlobalRunloopContext global_runloop_context;


According to Google C++ style guide https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables, static objects must be trivially destructible.

src/core/lib/iomgr/ev_apple.cc

guantaol · 2020-04-03T22:51:37Z

src/core/lib/iomgr/ev_apple.cc

+#endif  // NDEBUG
+
+struct GlobalRunloopContext {
+  grpc_core::Mutex mu;


Which fields are guarded by this mutex?

guantaol · 2020-04-03T22:57:01Z

src/core/lib/iomgr/ev_apple.cc

+  GrpcApplePollset* apple_pollset =
+      reinterpret_cast<GrpcApplePollset*>(pollset);
+  if (worker != nullptr) {
+    *worker = nullptr;


If you set worker to nullptr, how could you kick it later?

I think it works if pollset_kick broadcasts, according to comments here

What happens if there is a thread that do not want to be kicked by the broadcast? Previously this thread could simply set grpc_pollset_worker** worker == nullptr. Now all the thread using the same pollset will be waken up altogether.

muxi · 2020-04-07T17:12:07Z

@guantaol - updated. PTAL

muxi · 2020-04-09T16:39:25Z

@guantaol ping~

guantaol

I still need at least one other pass for this PR. Feel free to schedule a meeting to discuss this PR if that is more efficient.

guantaol · 2020-04-10T21:51:17Z

src/core/lib/iomgr/ev_apple.cc

@@ -0,0 +1,252 @@
+/*
+ *
+ * Copyright 2015 gRPC authors.


src/core/lib/iomgr/ev_apple.cc

guantaol · 2020-04-10T21:57:53Z

src/core/lib/iomgr/ev_apple.cc

+  grpc_core::CondVar init_cv;
+  grpc_core::CondVar input_source_cv;
+
+  // Protects all the variables below


This comment may not be true in the future if someone else adds a new member that is not protected by mu. May be better to use this style of comments (as other reviewers suggested to me in the past):

bool input_source_registered = false; /* GUARDED_BY(mu) */
...

When someday we actually introduce the GUARDED_BY annotation in grpc, it can be easily converted.

guantaol · 2020-04-10T23:21:07Z

src/core/lib/iomgr/ev_apple.cc

+static grpc_core::Thread* gGlobalRunLoopThread = nullptr;
+
+/// Register the stream with the dispatch queue. Callbacks of the stream will be
+/// issued to the dispatch queue and managed by Grand Central Dispatch.


When will the callback be issued, before or after read happens?

Added comment to explain in the code

guantaol · 2020-04-10T23:21:21Z

src/core/lib/iomgr/ev_apple.cc

+}
+
+/// Register the stream with the dispatch queue. Callbacks of the stream will be
+/// issued to the dispatch queue and managed by Grand Central Dispatch.


The same question here

Added comment to explain in the code

src/core/lib/iomgr/ev_apple.cc

guantaol · 2020-04-11T00:05:27Z

src/core/lib/iomgr/ev_apple.cc

+  GrpcApplePollset* apple_pollset =
+      reinterpret_cast<GrpcApplePollset*>(pollset);
+  if (worker != nullptr) {
+    *worker = nullptr;


What happens if there is a thread that do not want to be kicked by the broadcast? Previously this thread could simply set grpc_pollset_worker** worker == nullptr. Now all the thread using the same pollset will be waken up altogether.

guantaol · 2020-04-11T00:06:59Z

src/core/lib/iomgr/ev_apple.cc

+    *worker = nullptr;
+  }
+  while (!apple_pollset->kicked) {
+    if (apple_pollset->cv.Wait(


Why don't we need to grab the pollset mutex beforehand? If it is required to hold the mutex before calling pollset_work, this should be commented out.

It is exactly because it is required to hold the mutex before calling grpc_pollset_work. I don't really understand what you mean by "this should be commented out"

muxi · 2020-04-13T23:25:26Z

@guantaol - Updated. PTAL

guantaol · 2020-04-14T23:24:28Z

src/core/lib/iomgr/ev_apple.cc

+    *worker = reinterpret_cast<grpc_pollset_worker*>(&actual_worker);
+  }
+
+  if (apple_pollset->outstanding_kick) {


If this is the same as the kicked_without_poller in ev_epollex_linux, I suggest using the same name for easy understanding.

guantaol · 2020-04-14T23:28:11Z

src/core/lib/iomgr/ev_apple.cc

+
+  return GRPC_ERROR_NONE;
+}
+


Please add a comment to explain which mutex must be held before calling kick_worker().

muxi · 2020-04-15T18:07:38Z

Squashing commits

muxi added lang/ObjC release notes: yes Indicates if PR needs to be in release notes labels Mar 19, 2020

muxi requested review from gnossen, jtattermusch, nicolasnoble and veblush as code owners March 20, 2020 21:18

muxi force-pushed the objc-poller branch from 3d81729 to 0625a3a Compare March 20, 2020 21:28

muxi mentioned this pull request Mar 24, 2020

//test/cpp/end2end:nonblocking_test fails #22466

Closed

muxi force-pushed the objc-poller branch from 863ab93 to fb55cb3 Compare March 24, 2020 17:14

muxi added the kokoro:force-run label Mar 27, 2020

grpc-kokoro removed the kokoro:force-run label Mar 27, 2020

muxi requested a review from guantaol March 30, 2020 16:32

muxi added lang/core and removed lang/ObjC labels Mar 30, 2020

guantaol reviewed Apr 3, 2020

View reviewed changes

guantaol reviewed Apr 11, 2020

View reviewed changes

guantaol approved these changes Apr 14, 2020

View reviewed changes

Add apple_ev iomgr

b96895b

muxi force-pushed the objc-poller branch from 7bb1e5f to b96895b Compare April 15, 2020 18:07

muxi merged commit 7f36ff0 into grpc:master Apr 15, 2020

muxi deleted the objc-poller branch April 15, 2020 21:30

muxi mentioned this pull request Apr 16, 2020

Revert "Introduce CFRunLoop based iomgr" #22684

Merged

Conversation

muxi commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muxi commented Mar 20, 2020

Uh oh!

muxi commented Apr 2, 2020

Uh oh!

guantaol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxi commented Apr 7, 2020

Uh oh!

muxi commented Apr 9, 2020

Uh oh!

guantaol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxi commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muxi commented Apr 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

muxi commented Mar 19, 2020 •

edited

Loading