[coe] Remove gil when submit actor and tasks to avoid deadlock for some cases.

[fishbone]

Why are these changes needed?

When submit task, GIL is not released due to this PR.
This cause a potential deadlock when actor died and got notified by GCS. In this case, the callback function submitted by GetAsync is going to execute some python function and the GIL is still hold by submit task. And submit task is blocking by a lock which is not released.

In the previous PR, it seems to fix some memory issue, but it's seems not there any more.

Related issue number

#26414

Checks

• I've run scripts/format.sh to lint the changes in this PR.
• I've included any doc changes needed for https://docs.ray.io/en/master/.
• I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
• Testing Strategy
  • Unit tests
  • Release tests
  • This PR is not tested :(

[fishbone]

I tried to get a script to reproduce the issue, but it seems hard. Also, I actually can't find the root cause, backtrace is in here

The thing here is that submit task is holding GIL and trying to hold a mutex. The mutex should have been released by the other thread, but somehow not.

But just with this fix, the KubeRay HttpProxy won't hang there.

@edoakes are you think it's a safe fix? Do you know how can I verify the original issue is not there anymore?

[fishbone]

Also cc @scv119 please share some ideas if you have.

[edoakes]

@iycheng I read through the old thread and vaguely remember the context, but unfortunately I don't think I was ever able to get a minimal repo:

This fixes the issue by not releasing the GIL while calling SubmitActorTask (also included SubmitTask because it's likely to have the same issue). The fix was verified by manual testing using Serve's test_api.py. Unfortunately I don't fully understand the offending memory invalidation and I wasn't able to come up with a minimal regression test, but I think we should merge the fix regardless.

I would be pretty nervous about merging this PR as-is though given that we don't understand what would have fixed it independently...

[fishbone]

@edoakes finally I figure out what's the deadlock here. It's actually very simple, but the bad naming of the lock (we have a lot of mutex named as mu_ :( ) direct me to the wrong place.

Here when actor died, mu_ will be held and also later, it'll try to execute python function which require GIL

In SubmitActorTask, GIL is not released, and it tries to acquire mu_ and thus deadlock.

I don't think it can be easily fixed since in the disconnect actor, it actually need the mutex to be held. We probably should give this PR a try (whether nightly test or other test failed) first.

Let me see whether a test can be added.

[edoakes]

@iycheng I'm mostly following -- why does DisconnectActor need to acquire the GIL? Is it in the callbacks executed at the end of the function?

[edoakes]

Anyways, let's definitely run a full suite of nightly tests before merging.

[fishbone]

@iycheng I'm mostly following -- why does DisconnectActor need to acquire the GIL? Is it in the callbacks executed at the end of the function?

@edoakes yes, the callback contains a python function, so it'll need to acquire the GIL.

[edoakes]

Makes sense, Python callbacks have been a big source of deadlocks in the past. Would be great if we had a way to catch GIL-related deadlocks with absl somehow...

[fishbone]

Running here: https://buildkite.com/ray-project/release-tests-pr/builds/9465#018200d7-56d9-4926-8fe3-530358fe5f16

I'll keep an eye on the result.

[rkooo567]

There are many tests failing... In a glance, there should be no issues holding gil when calling c++ funcs... do you know why this was written this way in the first place? (wonder if we are missing any edge cases)

[fishbone]

There are many tests failing... In a glance, there should be no issues holding gil when calling c++ funcs... do you know why this was written this way in the first place? (wonder if we are missing any edge cases)

Please check the description. I believe it's because of memory corruption in some places before. (maybe cython bugs)

[fishbone]

Btw, @rkooo567 CI test failure is not related. Most nightly tests seems not related as well. I'll take another look.

[fishbone]

nightly tests are ok (either not related or failed in master). Merge this PR.