Skip to content

Commit

Permalink
Fix potential panics during panics (#2162)
Browse files Browse the repository at this point in the history
* Fix potential panics during panics

* Try to leak less

* Cosmetics
  • Loading branch information
marc0246 authored Mar 25, 2023
1 parent 5ab8691 commit 765bc6a
Show file tree
Hide file tree
Showing 7 changed files with 45 additions and 20 deletions.
3 changes: 2 additions & 1 deletion vulkano/src/buffer/subbuffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use std::{
ops::{Deref, DerefMut, Range, RangeBounds},
ptr::{self, NonNull},
sync::Arc,
thread,
};

pub use vulkano_macros::BufferContents;
Expand Down Expand Up @@ -637,7 +638,7 @@ impl<T: ?Sized> Drop for BufferWriteGuard<'_, T> {
BufferMemory::Sparse => unreachable!(),
};

if allocation.atom_size().is_some() {
if allocation.atom_size().is_some() && !thread::panicking() {
unsafe { allocation.flush_range(self.range.clone()).unwrap() };
}

Expand Down
8 changes: 8 additions & 0 deletions vulkano/src/command_buffer/allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use std::{
marker::PhantomData,
mem::ManuallyDrop,
sync::Arc,
thread,
};
use thread_local::ThreadLocal;

Expand Down Expand Up @@ -498,6 +499,11 @@ impl Pool {
impl Drop for Pool {
fn drop(&mut self) {
let inner = unsafe { ManuallyDrop::take(&mut self.inner) };

if thread::panicking() {
return;
}

unsafe { inner.inner.reset(false) }.unwrap();
inner.primary_allocations.set(0);
inner.secondary_allocations.set(0);
Expand Down Expand Up @@ -623,6 +629,8 @@ impl Drop for StandardCommandBufferAlloc {
CommandBufferLevel::Primary => &self.pool.inner.primary_pool,
CommandBufferLevel::Secondary => &self.pool.inner.secondary_pool,
};
// This can't panic, because if an allocation from a particular kind of pool was made, then
// the pool must exist.
let _ = pool.as_ref().unwrap().push(inner);
}
}
Expand Down
15 changes: 7 additions & 8 deletions vulkano/src/command_buffer/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use std::{
atomic::{AtomicBool, Ordering},
Arc,
},
thread,
};

pub unsafe trait PrimaryCommandBufferAbstract:
Expand Down Expand Up @@ -441,14 +442,12 @@ where
F: GpuFuture,
{
fn drop(&mut self) {
unsafe {
if !*self.finished.get_mut() {
// TODO: handle errors?
self.flush().unwrap();
// Block until the queue finished.
self.queue.with(|mut q| q.wait_idle()).unwrap();
self.previous.signal_finished();
}
if !*self.finished.get_mut() && !thread::panicking() {
// TODO: handle errors?
self.flush().unwrap();
// Block until the queue finished.
self.queue.with(|mut q| q.wait_idle()).unwrap();
unsafe { self.previous.signal_finished() };
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions vulkano/src/descriptor_set/allocator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::{
OomError,
};
use crossbeam_queue::ArrayQueue;
use std::{cell::UnsafeCell, mem::ManuallyDrop, num::NonZeroU64, sync::Arc};
use std::{cell::UnsafeCell, mem::ManuallyDrop, num::NonZeroU64, sync::Arc, thread};
use thread_local::ThreadLocal;

const MAX_POOLS: usize = 32;
Expand Down Expand Up @@ -417,7 +417,11 @@ impl VariablePool {
impl Drop for VariablePool {
fn drop(&mut self) {
let inner = unsafe { ManuallyDrop::take(&mut self.inner) };
// TODO: This should not return `Result`, resetting a pool can't fail.

if thread::panicking() {
return;
}

unsafe { inner.reset() }.unwrap();

// If there is not enough space in the reserve, we destroy the pool. The only way this can
Expand Down
11 changes: 10 additions & 1 deletion vulkano/src/swapchain/swapchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ use std::{
atomic::{AtomicBool, AtomicU64, Ordering},
Arc,
},
thread,
time::Duration,
};

Expand Down Expand Up @@ -1743,7 +1744,11 @@ unsafe impl GpuFuture for SwapchainAcquireFuture {

impl Drop for SwapchainAcquireFuture {
fn drop(&mut self) {
if let Some(ref fence) = self.fence {
if thread::panicking() {
return;
}

if let Some(fence) = &self.fence {
fence.wait(None).unwrap(); // TODO: handle error?
self.semaphore = None;
}
Expand Down Expand Up @@ -2184,6 +2189,10 @@ where
P: GpuFuture,
{
fn drop(&mut self) {
if thread::panicking() {
return;
}

unsafe {
if !*self.flushed.get_mut() {
// Flushing may fail, that's okay. We will still wait for the queue later, so any
Expand Down
5 changes: 5 additions & 0 deletions vulkano/src/sync/future/fence_signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use std::{
pin::Pin,
sync::Arc,
task::{Context, Poll},
thread,
time::Duration,
};

Expand Down Expand Up @@ -523,6 +524,10 @@ where
F: GpuFuture,
{
fn drop(&mut self) {
if thread::panicking() {
return;
}

let mut state = self.state.lock();

// We ignore any possible error while submitting for now. Problems are handled below.
Expand Down
15 changes: 7 additions & 8 deletions vulkano/src/sync/future/semaphore_signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use std::{
atomic::{AtomicBool, Ordering},
Arc,
},
thread,
};

/// Builds a new semaphore signal future.
Expand Down Expand Up @@ -250,14 +251,12 @@ where
F: GpuFuture,
{
fn drop(&mut self) {
unsafe {
if !*self.finished.get_mut() {
// TODO: handle errors?
self.flush().unwrap();
// Block until the queue finished.
self.queue().unwrap().with(|mut q| q.wait_idle()).unwrap();
self.previous.signal_finished();
}
if !*self.finished.get_mut() && !thread::panicking() {
// TODO: handle errors?
self.flush().unwrap();
// Block until the queue finished.
self.queue().unwrap().with(|mut q| q.wait_idle()).unwrap();
unsafe { self.previous.signal_finished() };
}
}
}

0 comments on commit 765bc6a

Please sign in to comment.