Use the correct top_ref when converting from streaming form #1223
Conversation
When skip_validate is true, SlabAlloc::attach_file() always returned 0 for the top_ref. This was fine when it's not the session initiator as the return value was ignored in favor of the ref from the lock file, but SharedGroup::compact() actually uses the return value.
|
Bummer! We introduced this error in PR #1201 a week ago when we fixed a related problem. Good catch. 👍 No doubt that attach_file is too overloaded/serves too many purposes. We've had far too many bugs here (either in attach_file or in functions using it such as SharedGroup::open and compact.) |
|
@kspangsege will you review this one as well? As we've had multiple bugs in this part of the code, we better play it safe. |
Too many different modes and potential combinations of modes, too. This change bandaids one broken combination, but I'm pretty sure there's more that just aren't used at the moment. |
|
@tg, @finnschiermer, for the sake of clarity I made some further modifications such that |
|
Coverage: 100% Please check your coverage here: https://ci.realm.io/job/core_matrix_pr/1112/buildcommand=jenkins-pipeline-coverage,buildtype=debug,encryption=no,slave=fastlinux/Diff_Coverage/ |
| Header* header = writable_map.get_addr(); | ||
| header->m_flags |= cfg.server_sync_mode ? flags_ServerSyncMode : 0x0; | ||
| Header& header = *writable_map.get_addr(); | ||
| header.m_flags |= cfg.server_sync_mode ? flags_ServerSyncMode : 0x0; |
There was a problem hiding this comment.
How about explicitly setting top_ref in this branch of the if as well?
There was a problem hiding this comment.
I think you misread the code here. Note that top_ref is already set in both cases, because it is set after the end of the else-block.
There was a problem hiding this comment.
My intention is certainly that top_ref must be set in every case, so let me know if you see any case where it fails.
There was a problem hiding this comment.
as far as I can see it is not set when did_create is true?
There was a problem hiding this comment.
Ah, got it. The closing-opening brace after the first condition confused me. I had to open it in the editor to understand the code :-)
Use the correct top_ref when converting from streaming form
When skip_validate is true, SlabAlloc::attach_file() always returned 0 for the top_ref. This was fine when it's not the session initiator as the return value was ignored in favor of the ref from the lock file, but SharedGroup::compact() actually uses the return value.