[Core] Fix plasma store segfault

[rkooo567]

Why are these changes needed?

It looks like when there are some pressure to the object store, this type of segfault rarely happens.

(raylet) [2021-04-01 14:17:37,987 E 65464 2851189] logging.cc:441: *** Aborted at 1617311857 (unix time) try "date -d @1617311857" if you are using GNU date ***
(raylet) [2021-04-01 14:17:37,987 E 65464 2851189] logging.cc:441: PC: @                0x0 (unknown)
(raylet) [2021-04-01 14:17:37,988 E 65464 2851189] logging.cc:441: *** SIGSEGV (@0x0) received by PID 65464 (TID 0x70000422a000) stack trace: ***
(raylet) [2021-04-01 14:17:37,993 E 65464 2851189] logging.cc:441:     @     0x7fff68fb35fd _sigtramp
(raylet) [2021-04-01 14:17:37,993 E 65464 2851189] logging.cc:441:     @     0x7fbe61910a80 (unknown)
(raylet) [2021-04-01 14:17:38,005 E 65464 2851189] logging.cc:441:     @        0x10644c36f plasma::PlasmaStore::ReturnFromGet()
(raylet) [2021-04-01 14:17:38,005 E 65464 2851189] logging.cc:441:     @        0x106452e89 boost::asio::detail::executor_function<>::do_complete()
(raylet) [2021-04-01 14:17:38,006 E 65464 2851189] logging.cc:441:     @        0x1062f7adb boost::asio::io_context::executor_type::dispatch<>()
(raylet) [2021-04-01 14:17:38,006 E 65464 2851189] logging.cc:441:     @        0x106452afe boost::asio::executor::dispatch<>()
(raylet) [2021-04-01 14:17:38,006 E 65464 2851189] logging.cc:441:     @        0x10645297b boost::asio::detail::wait_handler<>::do_complete()
(raylet) [2021-04-01 14:17:38,007 E 65464 2851189] logging.cc:441:     @        0x106c116a9 boost::asio::detail::scheduler::do_run_one()
(raylet) [2021-04-01 14:17:38,007 E 65464 2851189] logging.cc:441:     @        0x106c08a92 boost::asio::detail::scheduler::run()
(raylet) [2021-04-01 14:17:38,007 E 65464 2851189] logging.cc:441:     @        0x106c089bb boost::asio::io_context::run()
(raylet) [2021-04-01 14:17:38,008 E 65464 2851189] logging.cc:441:     @        0x1064571ac plasma::PlasmaStoreRunner::Start()
(raylet) [2021-04-01 14:17:38,008 E 65464 2851189] logging.cc:441:     @        0x1063f3c75 std::__1::__thread_proxy<>()
(raylet) [2021-04-01 14:17:38,008 E 65464 2851189] logging.cc:441:     @     0x7fff68fbf109 _pthread_start
(raylet) [2021-04-01 14:17:38,008 E 65464 2851189] logging.cc:441:     @     0x7fff68fbab8b thread_start

I looked into code, and it seems like we are using the raw pointer for GetRequest. It is highly likely the GetRequest is already GC'ed when ReturnFromGet is called. (Most likely due to this part of code)

    get_req->AsyncWait(timeout_ms, [this, get_req](const boost::system::error_code &ec) {
      if (ec != boost::asio::error::operation_aborted) {
        // Timer was not cancelled, take necessary action.
        ReturnFromGet(get_req);
      }
    });

This try fixing the issue by using the shared pointer in this path.

Related issue number

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 :(

[rkooo567]

@suquark Can you check if this seems like a valid approach to fix the issue? (We can also probably make ReturnFromGet no-op if get_request pointer is nullptr)

[suquark]

@rkooo567 I think make it noop is more reasonable. A request is deleted if

1. A client is disconnected.
2. ReturnFromGet is already executed.

Under both cases, we should not reply to the client anymore. If we do not make it a noop, then we could reply to the client twice.

BTW, I think the most important thing is to understand why there is a concurrency issue. I did not figure it out actually. Maybe the boost timer works in a way I do not expected.

[rkooo567]

I see. Good point. Do you mind if I make it no-op still with the shared pointer? If you'd like to keep the raw pointer, lmk. My approach will be like

1. instead of delete get_request, set the private attribute, e.g, get_request->MarkDeleted()
2. Make ReturnFromGet no op if it is set to be deleted.

Thought?

BTW, I think the most important thing is to understand why there is a concurrency issue. I did not figure it out actually. Maybe the boost timer works in a way I do not expected.

About this; I think this could be related? #15070

[suquark]

Yes! I think #15070 is the cause. It surprises me that boost timer cancellation is not an atomic operation (but actually reasonable considering the implementation of the eventloop).

We should use shared_ptr. The raw pointer comes from the original codebase that is written with pure C. Set the private attribute is good.

[rkooo567]

Didn't know it was written pure C first! Good to know...

[ericl]

LGTM

[rkooo567]

Failed window tests are failing in the master too.