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.

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

chore(storage): panic when LocalHummockStorage fail to send DestroyReadVersion event #9581

Closed
wants to merge 8 commits into from
29 changes: 22 additions & 7 deletions src/storage/src/hummock/event_handler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,19 +152,34 @@ pub struct LocalInstanceGuard {

impl Drop for LocalInstanceGuard {
fn drop(&mut self) {
// If sending fails, it means that event_handler and event_channel have been destroyed, no
// need to handle failure
// If sending fails, it means that event_handler and event_channel have been destroyed.
// Although the current assumption is that `LocalHummockStorage` is always Destroyed before
// `EventHandler`, so the send operation cannot fail. But when the assumptions are broken,
// this can lead to leaks. So prefer to panic, when it fails.

#[cfg(madsim)]
self.event_sender
.send(HummockEvent::DestroyReadVersion {
table_id: self.table_id,
instance_id: self.instance_id,
})
.unwrap_or_else(tracing::warn!(
"WARN LocalInstanceGuard table_id {:?} instance_id {} Drop SendError {:?}",
self.table_id,
self.instance_id,
err
));

#[cfg(not(madsim))]
self.event_sender
.send(HummockEvent::DestroyReadVersion {
table_id: self.table_id,
instance_id: self.instance_id,
})
.unwrap_or_else(|err| {
tracing::error!(
"LocalInstanceGuard table_id {:?} instance_id {} Drop SendError {:?}",
self.table_id,
self.instance_id,
err
panic!(
"ERROR LocalInstanceGuard table_id {:?} instance_id {} Drop SendError {:?}",
self.table_id, self.instance_id, err
)
})
}
Expand Down
25 changes: 19 additions & 6 deletions src/storage/src/hummock/local_version/pinned_version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,26 @@ impl PinnedVersionGuard {

impl Drop for PinnedVersionGuard {
fn drop(&mut self) {
if self
.pinned_version_manager_tx
#[cfg(madsim)]
self.pinned_version_manager_tx
.send(PinVersionAction::Unpin(self.version_id))
.is_err()
{
tracing::warn!("failed to send req unpin version id: {}", self.version_id);
}
.unwrap_or_else(|err| {
tracing::warn!(
"WARN failed to send req unpin version id: {} err {:?}",
self.version_id,
err
)
});

#[cfg(not(madsim))]
self.pinned_version_manager_tx
.send(PinVersionAction::Unpin(self.version_id))
.unwrap_or_else(|err| {
panic!(
"ERROR failed to send req unpin version id: {} err {:?}",
self.version_id, err
)
});
}
}

Expand Down