[event_engine] Introduce a event_engine_poller_for_python experiment by eugeneo · Pull Request #40243 · grpc/grpc

eugeneo · 2025-07-17T18:32:37Z

This commit introduces the event_engine_poller_for_python experiment, allowing for controlled rollout and testing of the EventEngine's Posix poller specifically within gRPC Python bindings.

Crucially, this change does not alter the default behavior for builds where GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER is not defined. In such configurations, the Posix EventEngine poller will continue to be enabled unconditionally, preserving existing functionality.

The primary impact of this change is when GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER is defined (e.g., in gRPC Python's build system). In this scenario, the enablement of the Posix poller transitions from being implicitly disabled to being configurable via the event_engine_poller_for_python experiment flag. This enables a controlled, experimental rollout of the EventEngine poller in environments that previously opted out of its direct instantiation.

drfloob · 2025-07-23T18:05:10Z

Discussed yesterday: let's split this PR up into (1) a cleanup/code reorganization PR, and (2) the changes required to introduce the experiment. Bundling them together means a rollback of both if there would otherwise only need to be a rollback of one.

src/core/client_channel/backup_poller.cc

drfloob · 2025-07-25T23:01:52Z

src/core/lib/event_engine/posix_engine/posix_engine.cc

  PosixError err;
  int connect_errno;
-  PosixEventPoller* poller = poller_manager_.Poller();
+  CHECK(poller_manager_.has_value());


This runtime check needed to be added 7 times, which feels like a code smell, and is error-prone if we forget to add it to any function (now or later). I know you're interested in removing PollerManager. If we're going to take this approach of having an optional PollerManager that is checked every time it's used, we need a short finite timeline before these checks are removed. Please add a TODO and explain what's needed before we can remove the need for this to be an optional type / remove the checks / delete the PollerManager.

drfloob · 2025-07-25T23:05:49Z

src/core/lib/event_engine/posix_engine/posix_engine.cc

    OnConnectCallback on_connect, const ResolvedAddress& addr,
    const EndpointConfig& args, MemoryAllocator memory_allocator,
    Duration timeout) {
+  CHECK(poller_manager_.has_value());


Won't this crash if the experiment is disabled?

It is supposed to crash. See CreateEndpointFromUnconnectedFdInternal. I just made it more explicit and sooner.

Co-authored-by: eugeneo <287917+eugeneo@users.noreply.github.com>

Vignesh2208

Mostly looks good. Left some minor comments

Vignesh2208 · 2025-08-29T21:02:56Z

src/core/lib/event_engine/shim.cc

 bool EventEngineExperimentDisabledForPython() {
 #ifdef GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER
-  return true;
+  return !grpc_core::IsEventEnginePollerForPythonEnabled();


nit: change the comment of this function. It doesn't always return true if GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER is defined.

Tried using Gemini to improve the comment...

Vignesh2208 · 2025-08-29T21:08:48Z

src/core/client_channel/backup_poller.cc

+#ifdef GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER
+  bool poller_enabled = grpc_core::IsEventEnginePollerForPythonEnabled();
+#else   // GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER
+  bool poller_enabled = true;


nit: rename local variable to event_engine_poller_enabled

Vignesh2208 · 2025-08-29T21:09:34Z

src/core/client_channel/backup_poller.cc


 void grpc_client_channel_global_init_backup_polling() {
-#ifndef GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER
+#ifdef GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER


nit: consider renaming this macro to GRPC_BUILD_FOR_PYTHON to make the intention clear

Need to discuss with Python team

Vignesh2208 · 2025-08-29T21:13:39Z

src/core/lib/event_engine/posix_engine/posix_engine.cc

-#endif  // GRPC_PLATFORM_SUPPORTS_POSIX_POLLING
-    }
+    AfterForkInChild();
+  }


Why is the check on IsEventEngineForkEnabled() removed ?

should there be an else if (poller_ != nullptr) { poller_->HandleForkInChild(); } here if the IsEventEngineForkEnabled() returned false ?

I added a CHECK.
Fork handlers should not be called if the fork is not enabled

This reverts commit b09b9cd.

sergiitk · 2025-09-08T21:55:03Z

Looks like this broke grpc/core/master/macos/grpc_basictests_python reconnect unittest. Only MacOS, only python 3.9.

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 592, in run
    self._callTestMethod(testMethod)
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 550, in _callTestMethod
    method()
  File "/Volumes/BuildData/tmpfs/altsrc/github/grpc/workspace_python_macos_opt_native/src/python/grpcio_tests/tests/unit/_reconnect_test.py", line 240, in test_reconnect
    self.fail(
  File "/Library/Frameworks/Python.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 676, in fail
    raise self.failureException(msg)
AssertionError: [TEST] Recovery verification failed: didn't receive 3 good calls in sequence. Failed calls: 16 out of 16

https://source.cloud.google.com/results/invocations/c1ba0a1f-f81a-48d6-9047-45cc88f09e0b

…eriment (grpc#40243)" This reverts commit 7949731.

…periment (grpc#40243)" This reverts commit 59e8124.

…rpc#40243) This commit introduces the `event_engine_poller_for_python` experiment, allowing for controlled rollout and testing of the EventEngine's Posix poller specifically within gRPC Python bindings. **Crucially, this change does *not* alter the default behavior for builds where `GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER` is *not* defined.** In such configurations, the Posix EventEngine poller will continue to be enabled unconditionally, preserving existing functionality. The primary impact of this change is when `GRPC_DO_NOT_INSTANTIATE_POSIX_POLLER` *is* defined (e.g., in gRPC Python's build system). In this scenario, the enablement of the Posix poller transitions from being implicitly disabled to being configurable via the `event_engine_poller_for_python` experiment flag. This enables a controlled, experimental rollout of the EventEngine poller in environments that previously opted out of its direct instantiation. Closes grpc#40243 COPYBARA_INTEGRATE_REVIEW=grpc#40243 from eugeneo:python-no-backup-poller-experiment 17906e6 PiperOrigin-RevId: 804472655

eugeneo requested a review from drfloob July 17, 2025 18:32

eugeneo assigned drfloob Jul 17, 2025

eugeneo requested a review from markdroth as a code owner July 17, 2025 18:32

eugeneo added the release notes: yes Indicates if PR needs to be in release notes label Jul 17, 2025

github-actions bot added the lang/core label Jul 17, 2025

grpc-checks bot added bloat/none per-call-memory/neutral per-channel-memory/neutral labels Jul 17, 2025

eugeneo force-pushed the python-no-backup-poller-experiment branch from d801812 to f578b7a Compare July 18, 2025 18:02

grpc-checks bot added per-call-memory/decrease per-call-memory/neutral and removed per-call-memory/neutral per-call-memory/decrease labels Jul 18, 2025

eugeneo force-pushed the python-no-backup-poller-experiment branch 3 times, most recently from c984ee6 to 8cae4bd Compare July 25, 2025 20:33

drfloob reviewed Jul 25, 2025

View reviewed changes

src/core/client_channel/backup_poller.cc Outdated Show resolved Hide resolved

drfloob reviewed Jul 25, 2025

View reviewed changes

eugeneo force-pushed the python-no-backup-poller-experiment branch from 8cae4bd to b4d011b Compare July 29, 2025 19:40

grpc-checks bot added bloat/low and removed bloat/none labels Jul 29, 2025

eugeneo force-pushed the python-no-backup-poller-experiment branch 8 times, most recently from 754c44b to 648631f Compare August 4, 2025 21:21

eugeneo force-pushed the python-no-backup-poller-experiment branch from 964aec9 to 3a0acf9 Compare August 14, 2025 21:12

github-actions bot and others added 2 commits August 14, 2025 14:31

Automated change: Fix sanity tests (#282)

3981472

Co-authored-by: eugeneo <287917+eugeneo@users.noreply.github.com>

fixup

141acf0

grpc-checks bot added bloat/none and removed bloat/low labels Aug 14, 2025

eugeneo assigned Vignesh2208 and unassigned drfloob Aug 15, 2025

eugeneo requested review from Vignesh2208 and removed request for markdroth August 15, 2025 17:54

fixup: crash less

56e2ea8

grpc-checks bot added bloat/low and removed bloat/none labels Aug 23, 2025

fixup: windows build

0318b91

Vignesh2208 reviewed Aug 29, 2025

View reviewed changes

Eugene Ostroukhov added 3 commits September 2, 2025 09:40

fixup: addressing comments

3b16161

fixup: make checks consistent

19f2006

fixup: rename macro

13fe746

github-actions bot added the lang/Python label Sep 2, 2025

Vignesh2208 approved these changes Sep 2, 2025

View reviewed changes

Eugene Ostroukhov added 4 commits September 3, 2025 10:17

fixup: make test pass

91d5d61

fixup

a203272

fixup: revert shims

b09b9cd

Revert "fixup: revert shims"

17906e6

This reverts commit b09b9cd.

copybara-service bot closed this in 7949731 Sep 8, 2025

eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Sep 9, 2025

Revert "[event_engine] Introduce a event_engine_poller_for_python exp…

59e8124

…eriment (grpc#40243)" This reverts commit 7949731.

eugeneo pushed a commit to eugeneo/grpc that referenced this pull request Sep 9, 2025

Reapply "[event_engine] Introduce a event_engine_poller_for_python ex…

020b397

…periment (grpc#40243)" This reverts commit 59e8124.

anniefrchz mentioned this pull request Oct 3, 2025

rekey anniefrchz/grpc#46

Closed

Conversation

eugeneo commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

drfloob commented Jul 23, 2025

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!

Vignesh2208 left a comment

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!

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!

sergiitk commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eugeneo commented Jul 17, 2025 •

edited

Loading