fix init rreq

[JacksonYao287]

we should do a deep copy for the user header , otherwise if the input user_header is freed after the call is returned, then m_header will become a dangling blob(the pointer in it is a dangling pointer).

why homestore start_replace_member/complete_replace_member(header involved) works well with this error

previously, we do not create and add rreq in to m_repl_key_req_map before propose_to_raft. the rreq is created and inserted into rreq when applier_create_req, where we created the rreq from the raft log entry, and the the in-memory log entry is available when the rreq#header is used, so the shallow copy works.

but now we create and add rreq in to m_repl_key_req_map before propose_to_raft.

so for follower , it works well since it is still created in applier_create_req

but for leader, the rreq , where the header blob is destroyed after we propose_to_raft. so the header of the rreq in m_repl_key_req_map will become a dangling blob.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.24%. Comparing base (1a0cef8) to head (cb51ae3).
⚠️ Report is 269 commits behind head on master.

Files with missing lines	Patch %	Lines
src/lib/replication/repl_dev/raft_repl_dev.cpp	0.00%	9 Missing ⚠️
src/include/homestore/replication/repl_dev.h	0.00%	2 Missing ⚠️
src/lib/replication/log_store/repl_log_store.cpp	0.00%	1 Missing ⚠️
...rc/lib/replication/repl_dev/raft_state_machine.cpp	0.00%	1 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #817      +/-   ##
==========================================
- Coverage   56.51%   55.24%   -1.27%     
==========================================
  Files         108      110       +2     
  Lines       10300    13278    +2978     
  Branches     1402     1934     +532     
==========================================
+ Hits         5821     7336    +1515     
- Misses       3894     5096    +1202     
- Partials      585      846     +261     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Besroy]

Thank you very much for your fix! I am considering whether we might encounter a similar issue when the follower saves push data before raft channel here: https://github.com/eBay/HomeStore/blob/master/src/lib/replication/repl_dev/raft_repl_dev.cpp#L942

[JacksonYao287]

I am considering whether we might encounter a similar issue when the follower saves push data before raft channel

@Besroy yes， but after this PR is merged, in applier_create_req , when init_req_ctx , we will also do a deep copy for the header. so this will not be a issue.

[xiaoxichen]

The intent of the fix is good. However it is very tricky to follow as the whole repl_ctx_t is all ptr based, for data, journal_buf, and every other field. Why the header need special cares....

The logic flow strarts from

1. we want to use init_req_ctx so that the quience state get horned for EGC.
2. The init_req_ctx creates rreq, we want to save it into the map
3. The buffer referenced is a local buffer, which can be released.
4. We need a deep copy of header, technically other fields need deep-copy as well, just we probabaly dont access them.

I would suggest break the flow from 2 or 3,
option 1: can we not save the rreq returned by init_req_ctx into map? Then later it will be recreated by using the journal buf.
option 2: similar to async_alloc_write etc, can we ensure the header_buf is live till the proposal committed ? i.e wait till the rreq is committed.

need @yuwmao 's input regarding wheter we should wait for step 3 completed before starting step 4. Also IIUC the step 4 (add member) wait till committed, that implies step 3 is commiited, then till the point of setp 3 committed the header is valid, why we are hitting issues here?

[JacksonYao287]

option 1: can we not save the rreq returned by init_req_ctx into map? Then later it will be recreated by using the journal buf.

init_req_ctx not only initializes header, key and journal, but also allocate local blks.
at follower side, rreq is created in applier_create_req(in raft_repl_dev#raft_event or raft_repl_dev#on_push_data_received) , where the local blk is also allocated. so , actually, option 1 is exact how follower works.

but for leader , it will be different. the key point is we need to propose_to_raft after the linked data is successfully written, and from journal we can recover header ,key, but can not allocate local blks as that in follower. we have to do it before propose_to_raft.

option 2: similar to async_alloc_write etc, can we ensure the header_buf is live till the proposal committed ? i.e wait till the rreq is committed.

this will involve additional user side logic to achieve this, not user friendly and likely lead to memory leak if not carefully handle this. so , on my end, i prefer not to do this , it better to do the deep copy by ourselves.

However it is very tricky to follow as the whole repl_ctx_t is all ptr based, for data, journal_buf, and every other field. Why the header need special cares....

yes, you are right, I think we need handle not only header, but also do a deep copy for key. for the journal_buf,
in leader, the buffer is allocated by the leader itself, not need to do a extra deep copy.

void repl_req_ctx::create_journal_entry(bool is_raft_buf, int32_t server_id) {
        ..........
        m_journal_buf = nuraft::buffer::alloc(entry_size);
        m_journal_entry = new (raft_journal_buf()->data_begin()) repl_journal_entry();

in follower, a move if used, which will move the ownership of the resource to rreq literally. need a double check

void repl_req_ctx::change_raft_journal_buf(raft_buf_ptr_t new_buf, bool adjust_hdr_key) {
    m_journal_buf = std::move(new_buf);
    m_journal_entry = r_cast< repl_journal_entry* >(raft_journal_buf()->data_begin());

[xiaoxichen]

The async_alloc_wrie is already working fine as the caller (HO ) will not relase the buffer. Now the problem is only for replcae member. Can we focuse on fixing these APIs as a fix, instead of a major API refactoring that assuming caller can release everything after propose_to_raft... I dont know if the later is even a goal for us,

Previously the replace member works fine as it doesnt insert the rreq into map at all and it magically works as it go through the applier_create_req on leader. This behavior is wired I agree.

[xiaoxichen]

Based on the business logic of replace member, it is likely the option 2 is naturally reached.

[JacksonYao287]

this error will affect all the apis that involves header, including start_replace_member/complete_replace_member and propose_truncate_boundary. (and potentially affect the apis that involves key in the future)

let me put more details here.

previously( before commit id c5d067f), for the above three log-only(no data written) and header-involved and apis, we create rreq for each api call. but this created rreq is only used as an input parameter of propose_to_raft, and it is not inserted in to req_map. instead, they will be recreated and inserted into the req_map when append log and localize_journal_entry_finish. here, the blobs of header and key of the newly created rreq points to the header and key in the in-memory log entry. this means if the in-memory log entry is destroyed , then the blobs of header and key will become dangling, this is a potential issue.

however, fortunately，the log entry is a shared_ptr, and we put it into the log cache when appending log, and can ensure the key and value blob is not dangling until the corresponding log entry is evicted. so , theoretically, if handle_commit of a rreq is executed before the log entry is evicted, then everything will go well.
this is how "it magically works" as you said.

but we made some changes recently and the behavior changes for leader. now, the rreq is created and inserted into req_map before propose_to_raft. so , for the above three apis, the header and key blob in rreq will definitely become dangling at leader(unless the caller can ensure they are valid).

Now the problem is only for replcae member. Can we focuse on fixing these APIs as a fix,

I got your point, but I think we should thoroughly address this issue, it is not only about a specific api, it is more about the rreq mechanism

[xiaoxichen]

Lets put solution aside and get a solid problem statement,

Any buffer given to rreq, including header/key/data, is volatile, meaning, rreq will NOT do any deep copy but relies on the owner of the buffer to sustain till the EOL of the rreq.

On follower,

• repl_req_ctx::change_raft_journal_buf moves owner of log buffer into rreq, that ensures header/key buffer in rreq is valid.
  On leader,
• For async_alloc_write, the caller(HO) holds the buffer till the request finished, no issue.
• For other operations mainly those log only start_replace_member/complete_replace_member/propose_truncate_boundary, the caller didnt hold the buffer properly, the behavior is subject to detail of HS, before commit id c5d067f, the rreq was created in exactly same flow as folloer which works fine. After this commit , we add the rreq into rreq_map which bypassed the localize_journal_entry_prepare.

[yuwmao]

Talked with @JacksonYao287 offline, replace_member related message header do need deep copy and the solution after commit id c5d067f looks good to me.
And given the intent of this pr is to unify the process of all types of messages on the leader side, can we check is_proposal and move the logic put rreq into m_repl_key_req_map into the RaftReplDev::init_req_ctx?

[JacksonYao287]

I set the behavior the same as before for non-async_alloc_write apis.
in proposer, all the rreqs of these apis will go through follower flow, which means they will be created and inserted into the map in ReplLogStore::append -> RaftStateMachine::localize_journal_entry_finish

let`s change this behavior later using a separate PR

[Besroy]

thanks for the fix!