[core] Posts CoreWorkerMemoryStore callbacks onto io_context. #47833

rynewang · 2024-09-27T00:30:06Z

CoreWorkerMemoryStore has GetAsync method which calls callbacks when the object is ready, immediately or on a Put() call. However despite the name, the call is not Async: it just calls on the GetAsync() or Put() call stack. If a mutex is held by any call frame in the callstack, it remains held in the callback, ripe for deadlocks.

Adds another ctor argument io_context and posts any callbacks to the context to avoid those deadlocks. Note many cpp tests creates CoreWorkerMemoryStore without a io_context so I kept the option of creating an owned io_context with thread.

Closes #47649

CoreWorkerMemoryStore has GetAsync method which calls callbacks when the object is ready, immediately or on a Put() call. However despite the name, the call is not `Async`: it just calls on the GetAsync() or Put() call stack. If a mutex is held by any call frame in the callstack, it remains held in the callback, ripe for deadlocks. Adds another ctor argument io_context and posts any callbacks to the context to avoid those deadlocks. Note many cpp tests creates CoreWorkerMemoryStore without a io_context so I kept the option of creating an owned io_context with thread. Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

…e task id Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao · 2024-10-07T22:08:31Z

@MengjinYan could you take a first pass.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

src/ray/core_worker/store_provider/memory_store/memory_store.cc

MengjinYan · 2024-10-08T20:16:39Z

src/ray/core_worker/transport/dependency_resolver.cc

Chatted with @rynewang offline. Here the PR changes the task id to request id because the corresponding tests (in dependency_resolver_test.cc) fail after the async change in memory store.

From the current understanding, the tests happen to use Tasks with the same TaskID to submit dependency resolve requests. With the async replication, the task state will be overridden and the tests will fail. At the same time, in reality, we are thinking although task execution can have multiple attempts, because the task object will be the same, so it might work with the original implementation.

Therefore, @jjyao we would like your opinion here to see whether the change here makes sense or we should just update the tests to make it more like real scenarios.

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-10-09T23:49:29Z

@jjyao pls take a look since Mengjin already reviewed

src/ray/core_worker/store_provider/memory_store/memory_store.cc

jjyao · 2024-10-11T22:14:55Z

src/ray/core_worker/transport/dependency_resolver.cc

@@ -59,11 +59,15 @@ void InlineDependencies(

 void LocalDependencyResolver::CancelDependencyResolution(const TaskID &task_id) {
  absl::MutexLock lock(&mu_);


Why changes in this file?

this is the change Mengjin mentioned. Problem is we used TaskID to track dependency resolution requests. But at least in cpp test, a same taskID may call ResolveDependencies multiple times, a new request overrides and deletes an old dependency resolution request. But the test still assumes the old request finishes with callback called.

This PR fixes it by tracing each request not only by TaskID but also by an auto increment id.

I am not sure if the current behavior (forgetting old reqs) is expected, so I want some input from you.

jjyao · 2024-10-11T22:16:07Z

src/ray/core_worker/transport/dependency_resolver.h

+  // ID of the dependency resolution request. Used to index pending_tasks_ because a
+  // same TaskID may be requested multiple times and we treat each request independently.


Is it a separate issue? Seems the PR description doesn't cover this change.

rynewang · 2024-10-25T21:28:16Z

todo: core_worker.cc GetAsync post removal

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-10-28T20:21:22Z

Friendly ping @jjyao

jjyao

LG

src/ray/core_worker/store_provider/memory_store/memory_store.cc

src/ray/core_worker/transport/dependency_resolver.h

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-10-29T17:39:18Z

@jjyao ready to merge

src/ray/core_worker/transport/dependency_resolver.cc

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang · 2024-10-29T23:33:15Z

@jjyao ready to review

jjyao · 2024-10-30T03:52:18Z

cpp/src/ray/runtime/object/local_mode_object_store.cc

@@ -26,8 +26,9 @@
 namespace ray {
 namespace internal {
 LocalModeObjectStore::LocalModeObjectStore(LocalModeRayRuntime &local_mode_ray_tuntime)
-    : local_mode_ray_tuntime_(local_mode_ray_tuntime) {
-  memory_store_ = std::make_unique<CoreWorkerMemoryStore>();
+    : io_context_("LocalModeObjectStore"),


Actually where this cause thread issue for cpp since now the callback is executed in a different thread?

I don't have an answer since I don't know ray-cpp that much

src/ray/core_worker/test/normal_task_submitter_test.cc

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

dentiny · 2024-10-30T23:52:45Z

src/mock/ray/core_worker/memory_store.h

+    std::unique_ptr<InstrumentedIOContextWithThread> io_context =
+        std::make_unique<InstrumentedIOContextWithThread>(
+            "DefaultCoreWorkerMemoryStoreWithThread");
+    return std::shared_ptr<DefaultCoreWorkerMemoryStoreWithThread>(


Why not std::make_shared? Your implementation leads to two memory allocations instead of one.

because the ctor is private and thus not visible to make_shared. you can make a private tag type trick but for sake of simplicity I did not do that.

…oject#47833) CoreWorkerMemoryStore has GetAsync method which calls callbacks when the object is ready, immediately or on a Put() call. However despite the name, the call is not `Async`: it just calls on the GetAsync() or Put() call stack. If a mutex is held by any call frame in the callstack, it remains held in the callback, ripe for deadlocks. Adds another ctor argument io_context and posts any callbacks to the context to avoid those deadlocks. Note many cpp tests creates CoreWorkerMemoryStore without a io_context so I kept the option of creating an owned io_context with thread. Signed-off-by: Ruiyang Wang <rywang014@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com> Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com>

rynewang mentioned this pull request Sep 27, 2024

[Core] ray.cancel deadlock #47649

Closed

fix cpp

c2f1d05

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang mentioned this pull request Sep 27, 2024

[Core] ray.cancel may be failed on an async actor method #46157

Open

cpp test

2224745

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

rynewang assigned Catch-Bull and rkooo567 Sep 27, 2024

rynewang added the go add ONLY when ready to merge, run all tests label Sep 27, 2024

rynewang added 5 commits October 3, 2024 22:21

fix cpp tests; make dependency_resolver safe on multiple calls of sam…

fd27c4a

…e task id Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

cpp test

1713040

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

fix bad capture

d2a4267

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

remove callback func copy

fbb065b

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

mutex protect mock class

7910b0b

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao assigned MengjinYan Oct 7, 2024

rynewang and others added 2 commits October 7, 2024 16:29

tsan

db8a11d

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Merge branch 'master' into async-mem-store

a8b6ecd

MengjinYan reviewed Oct 8, 2024

View reviewed changes

make io_context required

688cecb

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

MengjinYan assigned jjyao Oct 8, 2024

cpp

9bcbcdc

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao reviewed Oct 11, 2024

View reviewed changes

rynewang mentioned this pull request Oct 22, 2024

[core] Cancelling tasks who is waiting for dependencies hangs result objects #46315

Open

rynewang assigned rynewang and unassigned jjyao, MengjinYan, Catch-Bull and rkooo567 Oct 24, 2024

rynewang and others added 2 commits October 25, 2024 14:54

remove redaundant post in core worker

19779f4

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

Merge branch 'master' into async-mem-store

e61a6d3

jjyao reviewed Oct 28, 2024

View reviewed changes

rynewang and others added 4 commits October 28, 2024 21:53

Update src/ray/core_worker/store_provider/memory_store/memory_store.cc

8a2da3d

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

Update src/ray/core_worker/store_provider/memory_store/memory_store.cc

8891f38

Co-authored-by: Jiajun Yao <jeromeyjj@gmail.com> Signed-off-by: Ruiyang Wang <56065503+rynewang@users.noreply.github.com>

add comments for the post

5fdccac

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

remove RequestID, instead use vec<shared_ptr<>>

a1caf95

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao reviewed Oct 29, 2024

View reviewed changes

src/ray/core_worker/transport/dependency_resolver.cc Outdated Show resolved Hide resolved

src/ray/core_worker/transport/dependency_resolver.cc Outdated Show resolved Hide resolved

with random task id, revert multi reqs for 1 task id

49716d3

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao reviewed Oct 30, 2024

View reviewed changes

change wait 1s to wait for ioctx

2d5c238

Signed-off-by: Ruiyang Wang <rywang014@gmail.com>

jjyao approved these changes Oct 30, 2024

View reviewed changes

rynewang merged commit 500b3b6 into ray-project:master Oct 30, 2024
5 checks passed

rynewang deleted the async-mem-store branch October 30, 2024 17:48

dentiny reviewed Oct 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Posts CoreWorkerMemoryStore callbacks onto io_context. #47833

[core] Posts CoreWorkerMemoryStore callbacks onto io_context. #47833

rynewang commented Sep 27, 2024 •

edited by jjyao

Loading

jjyao commented Oct 7, 2024

MengjinYan Oct 8, 2024

rynewang commented Oct 9, 2024

jjyao Oct 11, 2024

rynewang Oct 12, 2024

jjyao Oct 11, 2024

rynewang commented Oct 25, 2024

rynewang commented Oct 28, 2024

jjyao left a comment

rynewang commented Oct 29, 2024

rynewang commented Oct 29, 2024

jjyao Oct 30, 2024

rynewang Oct 30, 2024

dentiny Oct 30, 2024

rynewang Oct 30, 2024

		@@ -59,11 +59,15 @@ void InlineDependencies(

		void LocalDependencyResolver::CancelDependencyResolution(const TaskID &task_id) {
		absl::MutexLock lock(&mu_);

		// ID of the dependency resolution request. Used to index pending_tasks_ because a
		// same TaskID may be requested multiple times and we treat each request independently.

[core] Posts CoreWorkerMemoryStore callbacks onto io_context. #47833

[core] Posts CoreWorkerMemoryStore callbacks onto io_context. #47833

Conversation

rynewang commented Sep 27, 2024 • edited by jjyao Loading

jjyao commented Oct 7, 2024

Choose a reason for hiding this comment

rynewang commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynewang commented Oct 25, 2024

rynewang commented Oct 28, 2024

jjyao left a comment

Choose a reason for hiding this comment

rynewang commented Oct 29, 2024

rynewang commented Oct 29, 2024

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

rynewang commented Sep 27, 2024 •

edited by jjyao

Loading