Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ISSUE #2188]💫Remove SyncUnsafeCellWrapper from mmapped_file🍻 #2189

Merged
merged 1 commit into from
Jan 10, 2025
Merged

[ISSUE #2188]💫Remove SyncUnsafeCellWrapper from mmapped_file🍻 #2189

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
use rocketmq_common::common::message::message_batch::MessageExtBatch;
use rocketmq_common::common::message::message_ext_broker_inner::MessageExtBrokerInner;
use rocketmq_common::UtilAll::ensure_dir_ok;
use rocketmq_rust::SyncUnsafeCellWrapper;
use tracing::debug;
use tracing::error;
use tracing::info;
Expand All @@ -58,7 +57,7 @@
pub struct DefaultMappedFile {
reference_resource: ReferenceResourceImpl,
file: File,
mmapped_file: SyncUnsafeCellWrapper<MmapMut>,
mmapped_file: MmapMut,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Confirmed thread-safety issues after SyncUnsafeCellWrapper removal

The verification reveals serious thread-safety concerns:

  • While metadata (positions, timestamps) is protected using atomic types, the actual memory-mapped file content lacks proper synchronization
  • get_mapped_file_mut() can create multiple mutable references to MmapMut, violating Rust's aliasing rules
  • Direct access to MmapMut without synchronization could lead to data races in concurrent scenarios

Consider wrapping MmapMut in a proper synchronization primitive like RwLock to ensure thread-safe access to the memory-mapped content.

🔗 Analysis chain

Verify thread-safety after removing SyncUnsafeCellWrapper.

The removal of SyncUnsafeCellWrapper might impact thread-safety. Ensure that all access to mmapped_file is properly synchronized through other mechanisms.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all concurrent access patterns to mmapped_file
rg -A 5 "mmapped_file" | rg -B 5 "async|thread|concurrent|parallel|sync|lock|mutex"

# Search for any synchronization mechanisms around MmapMut usage
ast-grep --pattern 'MmapMut'

Length of output: 701

Script:

#!/bin/bash
# Search for synchronization mechanisms around mmapped_file
rg -B 5 -A 10 "mmapped_file" rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs

# Look for method signatures and concurrent access patterns
ast-grep --pattern 'impl DefaultMappedFileImpl {
  $$$
  pub fn $_(&$_) {
    $$$
  }
  $$$
}'

# Search for Mutex/RwLock usage
rg "Mutex|RwLock" rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs

Length of output: 2497

transient_store_pool: Option<TransientStorePool>,
file_name: CheetahString,
file_from_offset: u64,
Expand Down Expand Up @@ -123,7 +122,7 @@
Self {
reference_resource: ReferenceResourceImpl::new(),
file,
mmapped_file: SyncUnsafeCellWrapper::new(mmap),
mmapped_file: mmap,
file_name,
file_from_offset,
mapped_byte_buffer: None,
Expand Down Expand Up @@ -205,7 +204,7 @@
start_timestamp: 0,
transient_store_pool: Some(transient_store_pool),
stop_timestamp: 0,
mmapped_file: SyncUnsafeCellWrapper::new(mmap),
mmapped_file: mmap,

Check warning on line 207 in rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs#L207 

Added line #L207 was not covered by tests
}
}
}
Expand Down Expand Up @@ -695,13 +694,14 @@
#[allow(unused_variables)]
impl DefaultMappedFile {
#[inline]
#[allow(clippy::mut_from_ref)]
pub fn get_mapped_file_mut(&self) -> &mut MmapMut {
self.mmapped_file.mut_from_ref()
unsafe { &mut *(self.mmapped_file.as_ptr() as *mut MmapMut) }

Check warning on line 699 in rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs#L699 

Added line #L699 was not covered by tests
}

#[inline]
pub fn get_mapped_file(&self) -> &MmapMut {
self.mmapped_file.as_ref()
unsafe { &*(self.mmapped_file.as_ptr() as *const MmapMut) }

Check warning on line 704 in rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs

View check run for this annotation

Codecov / codecov/patch

rocketmq-store/src/log_file/mapped_file/default_mapped_file_impl.rs#L704 

Added line #L704 was not covered by tests
Comment on lines +697 to +704
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix unsafe mutable reference handling.

The current implementation of get_mapped_file_mut violates Rust's aliasing rules by potentially creating multiple mutable references to the same data. Consider these alternatives:

  1. Use interior mutability with RefCell or Mutex
  2. Make the method require &mut self
  3. Document why the current unsafe implementation is sound

Additionally, get_mapped_file could be simplified to avoid unnecessary pointer casting.

Consider this safer implementation:

-#[allow(clippy::mut_from_ref)]
-pub fn get_mapped_file_mut(&self) -> &mut MmapMut {
-    unsafe { &mut *(self.mmapped_file.as_ptr() as *mut MmapMut) }
-}
+pub fn get_mapped_file_mut(&mut self) -> &mut MmapMut {
+    &mut self.mmapped_file
+}

-pub fn get_mapped_file(&self) -> &MmapMut {
-    unsafe { &*(self.mmapped_file.as_ptr() as *const MmapMut) }
-}
+pub fn get_mapped_file(&self) -> &MmapMut {
+    &self.mmapped_file
+}

Committable suggestion skipped: line range outside the PR's diff.

}

#[inline]
Expand Down
Loading