[EventEngine] RunAfter migration: grpc_chttp2_transport by yijiem · Pull Request #32240 · grpc/grpc

yijiem · 2023-01-30T22:28:20Z

No description provided.

with the help from refcount_solver.py :^)

drfloob

Looks pretty good. All of the extra UNREF_TRANSPORT calls are unfortunate, adds a bit of complexity to the ref counting if we need to debug problems, but I don't see a great way to avoid those at the moment.

src/core/ext/transport/chttp2/transport/internal.h

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

src/core/ext/transport/chttp2/transport/writing.cc

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

drfloob · 2023-02-01T17:56:24Z

src/core/ext/transport/chttp2/transport/writing.cc

-                        grpc_schedule_on_exec_ctx);
-      grpc_timer_init(&t->ping_state.delayed_ping_timer, next_allowed_ping,
-                      &t->retry_initiate_ping_locked);
+      t->ping_state.delayed_ping_timer_handle =


Could there be a race here between assignment of the handle and its usage in close_transport_locked? It might only be a problem in theory if so. I think next_allowed_ping - now would have to be near zero, and it would have to coincide with an immediately closed transport. So I'm guessing the likelihood is infinitesimally small, and that it would be very difficult to test for.

Hmmm, I didn't see the race... maybe_initiate_ping only runs in the transport's combiner lock, same for close_transport_locked I would think... So I don't think they will run concurrently or in parallel (correct me if I'm wrong though).

You're right. I didn't realize maybe_initiate_ping was only ever called with the combiner lock held.

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

yijiem

Thanks AJ!

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

src/core/ext/transport/chttp2/transport/internal.h

src/core/ext/transport/chttp2/transport/writing.cc

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

yijiem · 2023-02-02T01:30:43Z

src/core/ext/transport/chttp2/transport/writing.cc

-                        grpc_schedule_on_exec_ctx);
-      grpc_timer_init(&t->ping_state.delayed_ping_timer, next_allowed_ping,
-                      &t->retry_initiate_ping_locked);
+      t->ping_state.delayed_ping_timer_handle =


Hmmm, I didn't see the race... maybe_initiate_ping only runs in the transport's combiner lock, same for close_transport_locked I would think... So I don't think they will run concurrently or in parallel (correct me if I'm wrong though).

…tion_chttp2_transport

ctiller · 2023-02-08T04:22:57Z

src/core/ext/transport/chttp2/transport/chttp2_transport.cc

+          grpc_core::ApplicationCallbackExecCtx callback_exec_ctx;
+          grpc_core::ExecCtx exec_ctx;
+          absl::MutexLock lock(&*mu);
+          init_keepalive_ping(t);


Why not just re-enter the combiner here, eschewing the mutex?

yep, that's the other approach I was considering... init_keepalive_ping does re-enter the combiner, we need to run init_keepalive_pings_if_enabled in the combiner, so that init_keepalive_ping_locked is guaranteed to run after init_keepalive_pings_if_enabled had returned.

I decide to make the change since your prompt has changed my mind. :) PTAL. And also I don't like introducing mutex to chttp2_transport and adding performance overhead potentially.

This approach might slightly delay the run of the keepalive_ping_timer though.

I think that delay is probably ok, but I like that we keep a consistent locking story in this code (we want to change that story, and having uniformity ahead of time is a huge boon)

Gotcha, thanks Craig!

…)" This reverts commit 1b3eade.

) Reverts #32240 Breaks `grpc/core/master/macos/grpc_objc_bazel_test`, sample run: https://source.cloud.google.com/results/invocations/04e5a95b-5f06-4b3e-95fe-5e1895068475/targets Seems like the `testKeepaliveWithV2API` test case failed: ``` Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalCleartext testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' failed (5.547 seconds). ...... Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalSSL testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' failed (5.011 seconds). ```

* initial commit, bdp timer * migrate keepalive_ping_timer and keepalive_watchdog_timer * still has some issue with refcount * fix refcount with the help from refcount_solver.py :^) * migrate delayed_ping_timer * add some GPR_ASSERTs * comment * review * try mutex/lock * fix build deps * use GRPC_UNUSED * review

…c#32339) Reverts grpc#32240 Breaks `grpc/core/master/macos/grpc_objc_bazel_test`, sample run: https://source.cloud.google.com/results/invocations/04e5a95b-5f06-4b3e-95fe-5e1895068475/targets Seems like the `testKeepaliveWithV2API` test case failed: ``` Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalCleartext testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' failed (5.547 seconds). ...... Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalSSL testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' failed (5.011 seconds). ```

* initial commit, bdp timer * migrate keepalive_ping_timer and keepalive_watchdog_timer * still has some issue with refcount * fix refcount with the help from refcount_solver.py :^) * migrate delayed_ping_timer * add some GPR_ASSERTs * comment * review * try mutex/lock * fix build deps * use GRPC_UNUSED * review

) Reverts #32240 Breaks `grpc/core/master/macos/grpc_objc_bazel_test`, sample run: https://source.cloud.google.com/results/invocations/04e5a95b-5f06-4b3e-95fe-5e1895068475/targets Seems like the `testKeepaliveWithV2API` test case failed: ``` Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalCleartext testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalCleartext testKeepaliveWithV2API]' failed (5.547 seconds). ...... Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' started. <unknown>:0: error: -[InteropTestsLocalSSL testKeepaliveWithV2API] : Asynchronous wait failed: Exceeded timeout of 5 seconds, with unfulfilled expectations: "Keepalive". Test Case '-[InteropTestsLocalSSL testKeepaliveWithV2API]' failed (5.011 seconds). ```

yijiem added 6 commits January 26, 2023 20:48

initial commit, bdp timer

7b72085

migrate keepalive_ping_timer and keepalive_watchdog_timer

5cf037d

still has some issue with refcount

ee6cc44

fix refcount

37c0c5e

with the help from refcount_solver.py :^)

migrate delayed_ping_timer

c0e7773

add some GPR_ASSERTs

466109d

yijiem added the release notes: no Indicates if PR should not be in release notes label Jan 30, 2023

github-actions bot added the lang/core label Jan 30, 2023

grpc-checks bot added the bloat/low label Jan 30, 2023

yijiem added the kokoro:force-run label Jan 31, 2023

grpc-kokoro removed the kokoro:force-run label Jan 31, 2023

yijiem added the kokoro:force-run label Jan 31, 2023

grpc-kokoro removed the kokoro:force-run label Jan 31, 2023

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral labels Jan 31, 2023

comment

9f7a468

yijiem requested review from drfloob and yashykt January 31, 2023 18:35

yijiem marked this pull request as ready for review January 31, 2023 18:35

drfloob reviewed Feb 1, 2023

View reviewed changes

review

5094637

yijiem commented Feb 2, 2023

View reviewed changes

drfloob approved these changes Feb 6, 2023

View reviewed changes

yijiem added 4 commits February 7, 2023 15:42

try mutex/lock

92fa337

fix build deps

cc1a953

use GRPC_UNUSED

ca31d2b

Merge remote-tracking branch 'upstream/master' into ee_runafter_migra…

a596603

…tion_chttp2_transport

ctiller reviewed Feb 8, 2023

View reviewed changes

review

cae41b4

yijiem merged commit 1b3eade into grpc:master Feb 8, 2023

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Feb 8, 2023

yijiem added a commit that referenced this pull request Feb 9, 2023

Revert "[EventEngine] RunAfter migration: grpc_chttp2_transport (#32240…

599e407

…)" This reverts commit 1b3eade.

yijiem mentioned this pull request Feb 9, 2023

Revert "[EventEngine] RunAfter migration: grpc_chttp2_transport" #32339

Merged

Conversation

yijiem commented Jan 30, 2023

Uh oh!

drfloob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

yijiem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants